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: Add valid property AWS_SNS_TOPIC_ARN to AWS_SNS notification provider #783

Merged
merged 5 commits into from
Dec 10, 2021

Conversation

ghost
Copy link

@ghost ghost commented Dec 8, 2021

Notification integration fixes for #777

  • Fix AWS_SNS_TOPIC_ARN a valid argument for notification integration provider
  • Documentation, example and import

Test Plan

CGO_ENABLED=1 go test -race -coverprofile=coverage.txt -covermode=atomic ./...
? github.com/chanzuckerberg/terraform-provider-snowflake [no test files]
ok github.com/chanzuckerberg/terraform-provider-snowflake/pkg/datasources 0.802s coverage: 4.2% of statements
? github.com/chanzuckerberg/terraform-provider-snowflake/pkg/db [no test files]
ok github.com/chanzuckerberg/terraform-provider-snowflake/pkg/provider 0.602s coverage: 28.9% of statements
ok github.com/chanzuckerberg/terraform-provider-snowflake/pkg/resources 11.868s coverage: 55.0% of statements
ok github.com/chanzuckerberg/terraform-provider-snowflake/pkg/snowflake 0.454s coverage: 59.7% of statements
? github.com/chanzuckerberg/terraform-provider-snowflake/pkg/testhelpers [no test files]
ok github.com/chanzuckerberg/terraform-provider-snowflake/pkg/validation 0.249s coverage: 93.1% of statements
? github.com/chanzuckerberg/terraform-provider-snowflake/pkg/version [no test files]

  • acceptance tests
    Error notifications feature is currently in private preview link reference here and Snowflake staff need to enable it for that particular account before setting error_integration parameter would work.

References

https://docs.snowflake.com/en/LIMITEDACCESS/data-load-snowpipe-notifications.html#step-6-enabling-error-notifications-in-pipes

@ghost
Copy link
Author

ghost commented Dec 8, 2021

@alldoami could you please review and approve for Github Actions

@alldoami
Copy link
Contributor

alldoami commented Dec 8, 2021

Hmm I might be missing something but I only see name changes in this PR...

@ghost
Copy link
Author

ghost commented Dec 10, 2021

HI @alldoami,

Yes I raised the PR for name change of the variable provided. This is because of below error while creating the notification integration with terraform plan

SQL Query & Error

Query

CREATE NOTIFICATION INTEGRATION "test_notification_integration" AWS_SNS_ROLE_ARN='arn:aws:iam::accountid:role/name' AWS_SNS_ARN='arn:aws:sns:region:accountid:name' COMMENT='great comment' DIRECTION='OUTBOUND' NOTIFICATION_PROVIDER='AWS_SNS' TYPE='QUEUE' ENABLED=true;

Error

SQL compilation error: invalid property 'AWS_SNS_ARN' for 'INTEGRATION'

Test on snowflake account for proposed changes in this PR

  • After debugging, found the issue was with SQL query here.
  • Tested below SQL query successfully executed the variable name change

CREATE NOTIFICATION INTEGRATION "test_notification_integration" AWS_SNS_ROLE_ARN='arn:aws:iam::accountid:role/name' AWS_SNS_TOPIC_ARN='arn:aws:sns:region:accountid:name' COMMENT='great comment' DIRECTION='OUTBOUND' NOTIFICATION_PROVIDER='AWS_SNS' TYPE='QUEUE' ENABLED=true;

  • This query expects variable name as AWS_SNS_TOPIC_ARN instead of AWS_SNS_ARN

@alldoami
Copy link
Contributor

/ok-to-test sha=3dda35a

@github-actions
Copy link

Integration tests success for 3dda35a

Copy link
Contributor

@alldoami alldoami left a comment

Choose a reason for hiding this comment

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

I still think we need testing for this, so if you have time please write tests for this soon!

@alldoami alldoami merged commit 8224954 into Snowflake-Labs:main Dec 10, 2021
@ghost
Copy link
Author

ghost commented Dec 13, 2021

@alldoami could i request release for new version please.

daniepett pushed a commit to daniepett/terraform-provider-snowflake that referenced this pull request Feb 9, 2022
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.

None yet

2 participants