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

🪟 🐛 Fix custom connection creation endpoint #19702

Merged
merged 4 commits into from
Nov 23, 2022
Merged

🪟 🐛 Fix custom connection creation endpoint #19702

merged 4 commits into from
Nov 23, 2022

Conversation

josephkmh
Copy link
Contributor

@josephkmh josephkmh commented Nov 22, 2022 •
edited
Loading

What

Closes #19637

PR that introduced this bug was a precision revert of another issue: #19627

This PR removes the createSourceDefinition() and createDestinationDefinition() methods in favor of the new createCustomSourceDefinition() and createCustomDestinationDefinition() methods. createSourceDefinition() and createDestinationDefinition() are to be deprecated.

This PR does not address updating custom connectors. There is a backend issue for fixing this behavior here: #19669

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 22, 2022
@josephkmh josephkmh marked this pull request as ready for review November 22, 2022 10:20
@josephkmh josephkmh requested a review from a team as a code owner November 22, 2022 10:20
timroes
timroes approved these changes Nov 22, 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.

Code LGTM in accordance what we've discussed and outlined as a plan. Did not test locally, since the BE fix isn't in yet, so I'd anyway expect weird behavior still.

krishnaglick
krishnaglick reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

So the backend needs an update to correctly handle custom vs not? Because this looks like we're always creating custom now which was the issue before.

@josephkmh
Copy link
Contributor Author

josephkmh commented Nov 22, 2022 •
edited
Loading

@krishnaglick the issue before was actually only related to updating: non-custom connectors need to be updated with POST /v1/destination_definitions/update while custom connectors need to be updated with POST /v1/destination_definitions/update_custom. A separate backend ticket will fix this, where POST /v1/destination_definitions/update will update a connector whether it is custom or not.

Custom connectors should always be created with POST /v1/destination_definitions/create_custom, and POST /v1/destination_definitions/create should no longer be used in the webapp at all.

@krishnaglick
Copy link
Contributor

@krishnaglick the issue before was actually only related to updating: non-custom connectors need to be updated with POST /v1/destination_definitions/update while custom connectors need to be updated with POST /v1/destination_definitions/update_custom. A separate backend ticket will fix this, where POST /v1/destination_definitions/update will update a connector whether it is custom or not.

Custom connectors should always be created with POST /v1/destination_definitions/create_custom, and POST /v1/destination_definitions/create should no longer be used in the webapp at all.

My confusion stems from DestinationDefinitionService no longer having a non-custom endpoint as it was completely removed for the custom one. Is the codepath there always custom?

@josephkmh
Copy link
Contributor Author

My confusion stems from DestinationDefinitionService no longer having a non-custom endpoint as it was completely removed for the custom one. Is the codepath there always custom?

Yes exactly - creating a connector via the UI is now always a "custom" connector which is scoped to the current workspace.

krishnaglick
krishnaglick approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Have not tested locally, but code looks good.

@davinchia davinchia merged commit 8d4f7db into master Nov 23, 2022
@davinchia davinchia deleted the joey/fix-custom-connector-creation branch November 23, 2022 16:10
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 23, 2022
Closed
SofiiaZaitseva pushed a commit that referenced this pull request Nov 24, 2022
@josephkmh @SofiiaZaitseva
🪟 🐛 Fix custom connection creation endpoint (#19702)
0d83153 
Closes #19637

PR that introduced this bug was a precision revert of another issue: #19627

This PR removes the createSourceDefinition() and createDestinationDefinition() methods in favor of the new createCustomSourceDefinition() and createCustomDestinationDefinition() methods. createSourceDefinition() and createDestinationDefinition() are to be deprecated.

This PR does not address updating custom connectors. There is a backend issue for fixing this behavior here: #19669
letiescanciano added a commit that referenced this pull request Nov 28, 2022
@letiescanciano
Merge branch 'master' into leti/remove-empty-state-in-connector-setup
0b6fef1 
* master: (74 commits)
  Fix support icon in sidebar
  fix: add BuildPulse report for helm ac tests (#19785)
  fix: yaml syntax (#19775)
  🪟 🐛 Fix custom connection creation endpoint (#19702)
  Source facebook marketing: check "breakdowns" combinations (#19645)
  fix typo: notify instead of sync (#19737)
  find_non_rate_limited_PAT (#19736)
  Source Google Ads: fix schema for "campaigns" stream (#19700)
  🎉 Source Asana: migrate to new SAT, added base HTTP errors handling (#19561)
  fix order not to randomly fail backward compatibility check (#19377)
  Bump helm chart version reference to 0.42.0 (#19706)
  fix: add extraEnv block (#19703)
  Bump Airbyte version from 0.40.21 to 0.40.22 (#19687)
  Bump helm chart version reference to 0.41.3 (#19685)
  Add connector builder server to airbyte proxy, kube overlays, and helm charts (#19554)
  dbt Cloud integration doc (#19619)
  🪟 🎉 Display service token validation errors in the UI (#19578)
  17644 Update Destination data type test to use the new data types (#19281)
  Docs: fix broken connector builder UI docs links (#19631)
  Bump Airbyte version from 0.40.20 to 0.40.21 (#19634)
  ...
@github-actions github-actions bot mentioned this pull request Nov 28, 2022
Closed
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Nov 30, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krishnaglick krishnaglick krishnaglick 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
Development

Successfully merging this pull request may close these issues.

Fix creation of custom connector definitions
5 participants
@josephkmh @krishnaglick @timroes @davinchia @octavia-squidington-iv