Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jamkase/workspace slug #8734

Merged
merged 13 commits into from
Dec 16, 2021
Merged

Jamkase/workspace slug #8734

merged 13 commits into from
Dec 16, 2021

Conversation

jamakase
Copy link
Contributor

@jamakase jamakase commented Dec 12, 2021

What

Upgrade react-router to v6
Add redirect to the visited path in cloud version
Support workspace Slugs ( currently disabled and used ids instead )
Add ErrorBoundaries for absent resources

Comment indicators for connector update in cloud version

Closes #5227
Closes #7643
Closes #7721
Closes #6807

How

  • Add StartOverErrorView with ResourceNotFoundErrorBoundary.
  • Restructure application use relative routing from react-router v6

@github-actions github-actions bot added area/frontend area/platform issues related to the platform labels Dec 12, 2021
@github-actions github-actions bot added the area/connectors Connector related issues label Dec 13, 2021
@jamakase jamakase temporarily deployed to more-secrets December 13, 2021 13:58 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 13, 2021 17:24 Inactive
@jamakase jamakase self-assigned this Dec 13, 2021
@jamakase jamakase temporarily deployed to more-secrets December 13, 2021 20:27 Inactive
@github-actions github-actions bot removed the area/connectors Connector related issues label Dec 13, 2021
@jamakase jamakase temporarily deployed to more-secrets December 13, 2021 20:32 Inactive
@jamakase jamakase requested a review from jrhizor December 15, 2021 13:48
@jamakase jamakase marked this pull request as ready for review December 15, 2021 13:48
@jamakase jamakase temporarily deployed to more-secrets December 15, 2021 13:49 Inactive
@jrhizor
Copy link
Contributor

jrhizor commented Dec 16, 2021

I'm having trouble running this at all on docker-compose OSS.

When I started with an old installation (just rebuilt images) I got the following:
Screen Shot 2021-12-15 at 7 59 53 PM

I could see the sidebar, just going to http://localhost:8000/ redirected me to http://localhost:8000/, not an actual browsable page. If I go to http://localhost:8000//connections or other pages like that it works though.

However, if I start from scratch (not the old docker volumes) I still get redirected from http://localhost:8000/ to http://localhost:8000/ but I don't even see the sidebar:
Screen Shot 2021-12-15 at 8 02 48 PM

It also doesn't allow me to view anything at all and keeps on showing this page even on http://localhost:8000/36da2c49-861f-4fb4-8bb8-27d0c5a83afa/connections

I assume this is just because the initial setup wizard isn't set up for this.

My initial playing around looked good on my old Docker volumes except for these issues.

@jamakase jamakase temporarily deployed to more-secrets December 16, 2021 13:15 Inactive
@jamakase
Copy link
Contributor Author

@jrhizor indeed there was a problem with redirects: fixed it.

Also fixed unit tests for ServiceForm that were skipped for a long time.

@jamakase jamakase temporarily deployed to more-secrets December 16, 2021 13:38 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 16, 2021 17:37 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 16, 2021 17:46 Inactive
@jamakase jamakase temporarily deployed to more-secrets December 16, 2021 17:53 Inactive
Copy link
Contributor

@jrhizor jrhizor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Think it's mergable as is.

It would be nice if http://localhost:8000/doesnotexist showed as a 404 instead of "Unknown error occurred".

Also, some of the setting pages may end up on different routes eventually, since a lot of the settings are instance-wide and not workspace-specific. However I think it's worth it to just merge as is and figure that out later.

@jamakase
Copy link
Contributor Author

-> It would be nice if http://localhost:8000/doesnotexist showed as a 404 instead of "Unknown error occurred".

I think it's actually not necessary. e.g. github doesn't modify URL. I suppose seeing something like prod-airbyte.us-east-1.com/default-workspace/connection/pg-to-json makes much more sense than just having url/not-found

@jamakase jamakase merged commit 38ae962 into master Dec 16, 2021
@jamakase jamakase deleted the jamkase/workspace-slug branch December 16, 2021 23:38
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
* Workspace slug refactoring

* Migrate to react-router v6

* Update cloud routes

* Redirect after auth to initally visited route

* Remove redundant changes

* Add error boundaries for unfound resources

* Fix ServiceForm tests

* Fix redirects

* Fix e2e tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment