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

categorize source connectors based on types #6254

Closed
wants to merge 2 commits into from

Conversation

subodh1810
Copy link
Contributor

Issue : #6116

@subodh1810
Copy link
Contributor Author

I had a chat with @tuliren and he said the migration for updating the saved definitions is tricky and offered help to write the migration so that we can quickly get this across the finishing line.

Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

I think billing concerns shouldn't go in the source_definition table (especially given that this is OSS). But this particular piece of metadata seems ok since it can have other usecases around UX. In the future we should put any such concerns in a separate table IMO.

@tuliren
Copy link
Contributor

tuliren commented Sep 20, 2021

I had a chat with @tuliren and he said the migration for updating the saved definitions is tricky and offered help to write the migration so that we can quickly get this across the finishing line.

Yes. The co-existence of two migration systems makes it tricky.

Please do not merge this PR. I will work on this in the next hour.

@tuliren
Copy link
Contributor

tuliren commented Sep 20, 2021

I think that we may not need a migration at all:

  • The source_definitions.yaml is in airbyte-config:init, which is not a dependency for airybte-db:lib. If we don't introduce this dependency, the migration actually cannot read the YAML seed file.
  • Currently all the airbyte configs are just json blobs, so table structure is changed.
  • Each time the server is upgrades, we update the connector definition during server startup. We can leverage this process to add the new sourceType field to the json blob.

@tuliren
Copy link
Contributor

tuliren commented Sep 20, 2021

@subodh1810, I have completed the PR that adds the sourceType in this PR to the configs persistence: #6304.

Can you hold off on merging this PR? I think it is safer to merge the two PRs together. After #6304 is reviewed, and I have confirmed that these changes won't break any migration locally, I will merge yours first, and then mine. WDYT?

@subodh1810
Copy link
Contributor Author

subodh1810 commented Sep 20, 2021

Closing in favour of #6304

@subodh1810 subodh1810 closed this Sep 20, 2021
@swyxio swyxio deleted the categorise-connectors branch October 12, 2022 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants