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

Update source definition with new field #6304

Merged
merged 8 commits into from
Sep 20, 2021
Merged

Conversation

tuliren
Copy link
Contributor

@tuliren tuliren commented Sep 20, 2021

What

How

  • To add this new field, we need two things:
    • Make the StandardSourceDefinition schema forward compatible.
    • Update source definitions with this new field.

Recommended reading order

  1. DatabaseConfigPersistence.java
  2. The rest.

@tuliren tuliren temporarily deployed to more-secrets September 20, 2021 04:13 Inactive
@tuliren tuliren temporarily deployed to more-secrets September 20, 2021 05:01 Inactive
@tuliren tuliren temporarily deployed to more-secrets September 20, 2021 06:37 Inactive
@tuliren tuliren temporarily deployed to more-secrets September 20, 2021 08:54 Inactive
@tuliren tuliren marked this pull request as ready for review September 20, 2021 08:57
@@ -10,7 +10,7 @@ required:
- dockerRepository
- dockerImageTag
- documentationUrl
additionalProperties: false
additionalProperties: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be needed to make the file-based migration system forward compatible. I will test it locally if this is necessary.

Copy link
Contributor

@cgardens cgardens 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!

@@ -25,3 +25,10 @@ properties:
type: string
icon:
type: string
sourceType:
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be required?

Copy link
Contributor Author

@tuliren tuliren Sep 20, 2021

Choose a reason for hiding this comment

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

Probably not. This is in the OSS table, and we don't have preset values for custom connectors. So some entries won't have this.

Copy link
Contributor

@subodh1810 subodh1810 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Let me once this is ready to merge. As discussed over call, am closing my PR #6254 in favour of this as this one already has my changes

@tuliren
Copy link
Contributor Author

tuliren commented Sep 20, 2021

Confirmed locally that this change works as expected locally. Also there is no problem migrating from v0.17.0-alpha to the latest dev version.

@tuliren tuliren merged commit fef66e7 into master Sep 20, 2021
@tuliren tuliren deleted the liren/add-connector-category branch September 20, 2021 20:47
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.

3 participants