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.

Sign up for GitHub

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

Jump to bottom

🪟🔧 Use workspace-scoped endpoints for connector CRUD #19221

Merged
merged 7 commits into from
Nov 16, 2022
Merged

🪟🔧 Use workspace-scoped endpoints for connector CRUD #19221

merged 7 commits into from
Nov 16, 2022

Conversation

josephkmh
Copy link
Contributor

@josephkmh josephkmh commented Nov 9, 2022 •
edited
Loading

What

Closes #11606
Closes #11607

How

  • Updates API methods to use workspace-scoped endpoints
  • Fixes visual bug where loading state was not visible on CreateConnectorModal
  • Adds a migration to convert all existing custom connector definitions in oss deployments to be correctly configured as custom definitions based on the new schema changes

Recommended reading order

  1. src/core/domain/connector/DestinationDefinitionService.ts and src/core/domain/connector/SourceDefinitionService.ts contain the updated API methods
  2. src/services/connector/DestinationDefinitionService.ts and src/services/connector/SourceDefinitionService.ts contain the updated hooks
  3. I also fixed a small bug in CreateConnectorModal.tsx that I discovered while testing

🚨 User Impact 🚨

This PR includes a db migration that addresses #11607

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 9, 2022
@xiaohansong xiaohansong temporarily deployed to more-secrets November 11, 2022 00:33 Inactive
@xiaohansong xiaohansong requested a review from git-phu November 11, 2022 00:38
@xiaohansong
Copy link
Contributor

Adding @git-phu to review migration related changes. Thanks!

@josephkmh josephkmh changed the title 🪟🔧 [DRAFT] Use workspace-scoped endpoints for connector CRUD 🪟🔧 Use workspace-scoped endpoints for connector CRUD Nov 14, 2022
@josephkmh josephkmh requested a review from ambirdsall November 14, 2022 17:55
@josephkmh josephkmh marked this pull request as ready for review November 14, 2022 17:55
@josephkmh josephkmh requested a review from a team as a code owner November 14, 2022 17:55
@xiaohansong xiaohansong temporarily deployed to more-secrets November 14, 2022 19:21 Inactive
git-phu
git-phu approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor

@git-phu git-phu left a comment

Choose a reason for hiding this comment

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

migration looks good!

...byte/db/instance/configs/migrations/V0_40_18_004__BackfillActorDefinitionWorkspaceGrant.java Show resolved Hide resolved
timroes
timroes approved these changes Nov 16, 2022
View reviewed changes
Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Reviwed FE code. Code LGTM. Have not tested locally.

@xiaohansong xiaohansong merged commit 43feacc into master Nov 16, 2022
@xiaohansong xiaohansong deleted the joey/workspace-scoped-connector-definitions branch November 16, 2022 19:08
ambirdsall
ambirdsall reviewed Nov 16, 2022
View reviewed changes
Copy link
Contributor

@ambirdsall ambirdsall left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seeing this just got merged; while it's too late to officially approve, I did read through the code and was set to approve as soon as I double-checked a few things in a local checkout this morning. Sorry for being so delayed; for posterity, everything looks good to me.

@josephkmh
Copy link
Contributor Author

All good, thanks!

@timroes timroes mentioned this pull request Nov 16, 2022
Merged
@github-actions github-actions bot mentioned this pull request Nov 17, 2022
Merged
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 18, 2022
Merged
krishnaglick added a commit that referenced this pull request Nov 18, 2022
@krishnaglick
Revert "🪟🔧 Use workspace-scoped endpoints for connector CRUD (#19221)"
54439cd 
This reverts commit 43feacc.
This was referenced Nov 18, 2022
Closed
Merged
@josephkmh josephkmh mentioned this pull request Nov 20, 2022
Closed
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
@josephkmh @xiaohansong @akashkulk
🪟🔧 Use workspace-scoped endpoints for connector CRUD (#19221)
efdf69b 
* fix button loading state when creating connector

* use workspace scoped endpoints

* get and update methods

* backfill script

* commit new migration file

Co-authored-by: Xiaohan Song <xiaohan@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambirdsall ambirdsall ambirdsall left review comments

@xiaohansong xiaohansong xiaohansong left review comments

@git-phu git-phu git-phu approved these changes

@timroes timroes timroes approved these changes

Assignees
No one assigned
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Milestone
No milestone
5 participants
@josephkmh @xiaohansong @timroes @git-phu @ambirdsall