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

Fix creation of custom connector definitions #19637

Closed
josephkmh opened this issue Nov 20, 2022 · 6 comments · Fixed by #19702
Closed

Fix creation of custom connector definitions #19637

josephkmh opened this issue Nov 20, 2022 · 6 comments · Fixed by #19702
Assignees
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/compose

Comments

@josephkmh
Copy link
Contributor

josephkmh commented Nov 20, 2022

When creating a custom source/destination connector, the frontend should use createCustomSourceDefinition() and createCustomDestinationDefinition() respectively. The currently used methods (createSourceDefinition() and createDestinationDefinition()) result in orphaned rows in the actor_catalog.

Context on what led to this issue:

PR 19221 updated the following API methods in the webapp globally:

  1. createSourceDefinition() => createCustomSourceDefinition()
  2. createDestinationDefinition() => createCustomDestinationDefinition()
  3. updateDestinationDefinition() => updateCustomDestinationDefinition()
  4. updateSourceDefinition() => updateCustomSourceDefinition()

While 1. and 2. were correct, 3. and 4. were causing OSS users to no longer be able to update connectors that are not custom.

This resulted in an oncall issue. A targeted revert PR was introduced (PR 19627) that reverted all 4 methods above. This means:

  1. Non-custom connectors can now be updated again (good!)
  2. New custom connectors are now being created with the old methods again (impact unclear, probably a new migration similar to the first one will be needed to update these)
  3. Custom connectors created before the revert commit are now being updated with the old methods (unclear whether this works, needs to be tested)

Relates to #9652

@josephkmh josephkmh added area/platform issues related to the platform team/compose area/frontend Related to the Airbyte webapp labels Nov 20, 2022
@josephkmh josephkmh self-assigned this Nov 20, 2022
@octavia-squidington-iv
Copy link

cc @airbytehq/frontend

@josephkmh
Copy link
Contributor Author

After some local testing: creating custom connectors on master currently does not work. The connectors are created in the database, but are not returned by the server and consequently not shown in the UI. This is because the webapp is now using the old createSourceDefinition() endpoint, and the corresponding row in actor_definition_workspace_grant is not created.

@timroes
Copy link
Collaborator

timroes commented Nov 21, 2022

Synced offline about this. Our outcome and action items

  • createCustomSource/DestinationDefinition should be used (with workspaceId)
  • Once in UI, we want to remove the old (non workspace scoped) create APIs
  • updateSource/DestinationDefinition (does not need workspaceId)
    • Cloud will check if the passed connector ID, is actually assigned to a workspace that the user has access to, if not throws 404 in cloud if connector ID
  • Same logic as for update should apply for the delete endpoint
  • No migration will be needed, we accept orphaned rows in the actor table for the rare cases users ran into that case

@xiaohansong
Copy link
Contributor

xiaohansong commented Nov 21, 2022

On backend we need to implement following tickets to address these action items:

#19669
#19670

@josephkmh josephkmh changed the title Differentiate between custom and non-custom connectors when updating Fix creation of custom connector definitions Nov 22, 2022
@josephkmh
Copy link
Contributor Author

Thanks @xiaohansong - could we also add the removal of the old creation endpoints to #19669 (or a new issue)?

  • POST /v1/source_definitions/create
  • POST /v1/destination_definitions/create

Just want to make sure we get rid of those two endpoints so we don't have another accidental usage of them in the FE again 🙂

@xiaohansong
Copy link
Contributor

@josephkmh Just did that!

davinchia pushed a commit that referenced this issue Nov 23, 2022
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
SofiiaZaitseva pushed a commit that referenced this issue Nov 24, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/compose
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants