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

Remove default value from namespaceDefinitionType #17177

Merged
merged 4 commits into from
Sep 26, 2022

Conversation

pmossman
Copy link
Contributor

@pmossman pmossman commented Sep 26, 2022

What

Fixes a bug where the Destination Namespace can be reset to the default of 'source'.

Now that we can send partial updates for Connections, we need to remove the default value from namespaceDefinitionType so that partial updates that leave this value alone aren't defaulted to an unintended value.

How

  • Remove the 'default' property from NamespaceDefinitionType
  • Add in logic to default to 'source' to the ConnectionCreation handler. This way, creation behavior should remain the same, while update behavior supports partial updates that leave the NamespaceDefinitionType unchanged

@github-actions github-actions bot added area/api Related to the api area/platform issues related to the platform area/server labels Sep 26, 2022
@pmossman pmossman temporarily deployed to more-secrets September 26, 2022 21:13 Inactive
@pmossman pmossman force-pushed the parker/remove-default-namespace-format branch from 0b3ae36 to a77e2ea Compare September 26, 2022 22:13
@pmossman pmossman marked this pull request as ready for review September 26, 2022 22:13
@pmossman pmossman temporarily deployed to more-secrets September 26, 2022 22:15 Inactive
Copy link
Contributor

@mfsiega-airbyte mfsiega-airbyte left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the acceptance tests look good!

@pmossman
Copy link
Contributor Author

The failing acceptance test looks to be unrelated, it's been failing in master for a while, so I'm going to merge this in order to unblock an OC fix

@pmossman pmossman merged commit e30476e into master Sep 26, 2022
@pmossman pmossman deleted the parker/remove-default-namespace-format branch September 26, 2022 22:43
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
* remove default value from namespaceDefinitionType

* fix test

* switch some tests to use PATCH-style connection update

* add default logic to connection creation

Co-authored-by: Michael Siega <michael@airbyte.io>
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* remove default value from namespaceDefinitionType

* fix test

* switch some tests to use PATCH-style connection update

* add default logic to connection creation

Co-authored-by: Michael Siega <michael@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants