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

Add new 'api integration' and 'external function' resources #491

Merged

Conversation

asaintsever
Copy link
Contributor

  • Add support for new snowflake_api_integration and snowflake_external_function resources
  • Add unit and acceptance tests
  • Add examples & updated documentation

Test Plan

Changes have been compiled and tested successfully (including acceptance tests)

  • unit tests
  • acceptance tests

References

@asaintsever asaintsever requested a review from a team as a code owner March 10, 2021 13:05
@asaintsever
Copy link
Contributor Author

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.
Best.

@alldoami
Copy link
Contributor

Sorry to do this, but would it be possible to separate this PR into two PRs? It is a lot to review 😅

@asaintsever
Copy link
Contributor Author

@alldoami
Well, it doesn't really make sense to split this contribution into several PR. If not already done, I would suggest you read https://docs.snowflake.com/en/sql-reference/external-functions-introduction.html : it is not possible to create an external function without creating an api integration. They come both at play. For distinct and not related resources, I would have submitted different PR of course but not the case here.

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?

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.

Few NITs

pkg/resources/api_integration.go Show resolved Hide resolved
pkg/resources/api_integration.go Show resolved Hide resolved
pkg/resources/api_integration.go Show resolved Hide resolved
@alldoami
Copy link
Contributor

/ok-to-test sha=97cec08

@github-actions
Copy link

Integration tests success for 97cec08

@alldoami
Copy link
Contributor

/ok-to-test sha=7e124a9

@github-actions
Copy link

Integration tests success for 7e124a9

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'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.

@asaintsever
Copy link
Contributor Author

asaintsever commented Mar 19, 2021

That's true. It turns out that using quotes works fine as it is the case with any query arg/param.
Yet, the documentation states that API_PROVIDER "should not be in quotation marks.". I'll fix that to comply with this requirement.

@alldoami
Copy link
Contributor

/ok-to-test sha=ba8097d

@github-actions
Copy link

Integration tests success for ba8097d

@alldoami
Copy link
Contributor

@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.

@asaintsever
Copy link
Contributor Author

asaintsever commented Mar 24, 2021

@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 STORAGE_PROVIDER and TYPE with quotes in the final SQL request whereas this is not the case in the examples (https://docs.snowflake.com/en/sql-reference/sql/create-storage-integration.html#examples). Again, this is allowed, depends if you want case-sensitive identifiers or not.

@alldoami
Copy link
Contributor

/ok-to-test sha=ff46096

@github-actions
Copy link

Integration tests success for ff46096

@alldoami alldoami merged commit f250b80 into Snowflake-Labs:main Mar 24, 2021
anton-chekanov pushed a commit to anton-chekanov/terraform-provider-snowflake that referenced this pull request Jan 25, 2022
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