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: snowpipe error integration #2227

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Conversation

sonya
Copy link
Contributor

@sonya sonya commented Dec 1, 2023

This PR fixes a bug where setting error_integration on a snowflake_pipe resource would cause the name of the integration to be written to the comment field on the pipe instead of the error integration. A separate bug prevented the error_integration diffs from being detected if an SNS topic was set.

Test Plan

I tested this using an existing stack where I have many pipes. I made an earnest attempt to update pipes_integration_test.go, but I reached the point where I needed a real SNS topic and decided that was beyond the timebox I set for this.

Using version 0.76.0:

  1. Started with already deployed pipes that had a comment but no error_integration
  2. Added error_integration to the terraform config.
  3. terraform apply
  4. Ran describe pipe on the updated pipes in Snowflake. The comments now contained the name of the notification integration, whereas error_integration was null.
  5. terraform apply again with no changes
  6. Ran describe pipe on the updated pipes in Snowflake. The comments were back to their previous value, but error_integration was still null.
  7. Removed error_integration from the terraform config.
  8. terraform apply
  9. Ran describe pipe on the updated pipes in Snowflake. The comments were now blank.

Using the modified provider

  1. Started with pipes that had a comment but no error_integration
  2. Added error_integration to the terraform config.
  3. terraform apply
  4. Ran describe pipe on the updated pipes in Snowflake. The comments were unchanged and error integrations were set to the expected value.
  5. Removed error_integration from the terraform config
  6. terraform apply
  7. Ran describe pipe on the updated pipes in Snowflake. The comments were unchanged, and error integrations were null.

References

I initially started to file a bug report, but during investigation I figured out how to fix it so I'm opening a PR instead. Let me know if this isn't the right process

@sfc-gh-asawicki
Copy link
Collaborator

Hey @sonya. Thanks for the contribution!

We will review it in the upcoming days. cc: @sfc-gh-jcieslak @scottwinkler

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak 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 contributing, looks good to me 👍

@@ -56,6 +56,7 @@ type PipeSet struct {
}

type PipeUnset struct {
ErrorIntegration *bool `ddl:"keyword" sql:"ERROR_INTEGRATION"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick question: documentation does not list ErrorIntegration as a possible value for UNSET https://docs.snowflake.com/en/sql-reference/sql/alter-pipe#parameters. I guess that this is an error in the docs, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I checked it and it works

@sfc-gh-asawicki
Copy link
Collaborator

/ok-to-test sha=3c8f11d1fb023894a04dd4e696fe61f446d0db3b

Copy link

github-actions bot commented Dec 1, 2023

Integration tests failure for 3c8f11d1fb023894a04dd4e696fe61f446d0db3b

@sfc-gh-asawicki sfc-gh-asawicki merged commit 0b388bf into Snowflake-Labs:main Dec 1, 2023
5 of 7 checks passed
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