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

allow src/dest validation to be skipped for tombstone entries #17181

Merged
merged 8 commits into from
Sep 27, 2022

Conversation

colesnodgrass
Copy link
Member

What

How

  • update private method statefulUpdateSecrets to accept an additional boolean validate parameter which is used to determine if JsonSchemaValidator.ensure should be called
  • add tests to validate that JsonSchemaValidator.ensure is called correctly

Recommended reading order

  1. x.java

@colesnodgrass colesnodgrass temporarily deployed to more-secrets September 26, 2022 22:45 Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets September 26, 2022 22:52 Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets September 26, 2022 22:59 Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets September 26, 2022 23:10 Inactive

final SplitSecretConfig splitSecretConfig;
if (oldConfig.isPresent()) {
splitSecretConfig = SecretsHelpers.splitAndUpdateConfig(
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like the conditional logic should be internal to the SecretsHelper. Otherwise, it requires the caller to know when to use the different methods. Because the oldConfig is an optional, the logic to determine how to split the config could be codified inside the method, simplifying the call from the client code (e.g. here).

Copy link
Member Author

Choose a reason for hiding this comment

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

The only part that differs logically is the method from before are these three lines:

    if (validate) {
      validator.ensure(spec.getConnectionSpecification(), fullConfig);
    }

Everything else, including the section you called out, was existing logic that I slightly cleaned up to remove some duplicate code. I agree with what you're saying though and will make that change as part of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@colesnodgrass No worries...it was hard to tell if it was pre-existing. Fine to do that cleanup in a follow-on PR if that's easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdpgrailsdev I think I will do it in a follow-up PR, I started pulling on this thread to clean this up and it started pulling in other unrelated code as well.

@colesnodgrass colesnodgrass temporarily deployed to more-secrets September 27, 2022 19:28 Inactive
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@colesnodgrass colesnodgrass merged commit 9025e31 into master Sep 27, 2022
@colesnodgrass colesnodgrass deleted the cole/16113-non-delete-on-config-error branch September 27, 2022 20:22
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
…ehq#17181)

* allow src/dest validation to be skipped for tombstone entries

* formatting

* formatting again

* formatting again x2

* formatting again x3
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
…ehq#17181)

* allow src/dest validation to be skipped for tombstone entries

* formatting

* formatting again

* formatting again x2

* formatting again x3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot delete a source due to configuration error
3 participants