-
Notifications
You must be signed in to change notification settings - Fork 418
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
Add new 'api integration' and 'external function' resources #491
Add new 'api integration' and 'external function' resources #491
Conversation
Hi @edulop91, Is there something more I should provide to have this PR considered? I saw some PR rotting since several months and I am keen to have those changes merged as we are already making use of these new resources in Terraform configurations. Thanks in advance for your guidance. |
Sorry to do this, but would it be possible to separate this PR into two PRs? It is a lot to review 😅 |
@alldoami Moreover, by splitting into 2 PR the risk is to merge one and not the other. So you'll have to review both in the end and make sure both are part of same release. Is it what you want? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few NITs
/ok-to-test sha=97cec08 |
Integration tests success for 97cec08 |
…ntsever/terraform-provider-snowflake into asaintsever/feat/external_functions
/ok-to-test sha=7e124a9 |
Integration tests success for 7e124a9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking at the syntax docs on the API Integration and it looks like the API_PROVIDER
value should not be in quotes: https://docs.snowflake.com/en/sql-reference/sql/create-api-integration.html#syntax, I think the statement you query wraps the value in quotes.
That's true. It turns out that using quotes works fine as it is the case with any query arg/param. |
/ok-to-test sha=ba8097d |
Integration tests success for ba8097d |
@asaintsever I believe this comment is the same for the external function: #491 (review), seeing it here in the example: https://docs.snowflake.com/en/sql-reference/sql/create-external-function.html#examples. |
@alldoami sorry I do not understand. What do you mean? As for identifiers, Snowflake accepts both quoted and unquoted forms: https://docs.snowflake.com/en/sql-reference/identifiers-syntax.html So, except for the API_PROVIDER that should be unquoted as stated by the documentation (even if it works with quotes ...), you are free to have quotes or not. Moreover, if I look at existing resources like Storage Integration (https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/main/pkg/resources/storage_integration.go), I see |
/ok-to-test sha=ff46096 |
Integration tests success for ff46096 |
snowflake_api_integration
andsnowflake_external_function
resourcesTest Plan
Changes have been compiled and tested successfully (including acceptance tests)
References