fix: external function varchar return type validation

[nijave]

Fixes #3392

[sfc-gh-jmichalak]

LGTM with a few notes (we can address this on our side):

• add an entry in the migration guide
• add an acceptance test (for VARCHAR with and without parameter)
• (nit) update the comment above the change

[sfc-gh-jmichalak]

/ok-to-test sha=e8f70e390b28b28a9a4f4781fe91fdf59f08590f

[github-actions]

Integration tests failure for e8f70e390b28b28a9a4f4781fe91fdf59f08590f

[sfc-gh-asawicki]

I would like to add to what @sfc-gh-jmichalak wrote. We need to validate the previous behavior and the proposed correctness (that's why we require at least acceptance tests). Ideally, we should address all the data types that are potentially affected (I doubt that VARCHAR is the only invalid one here). Because of that, adding an integration test to the DESCRIBE behavior should be added to the tests set (similar to e.g.

terraform-provider-snowflake/pkg/sdk/testint/functions_integration_test.go

Line 1981 in 9be2196

for _, tc := range []string{

). This is a preview resource and we won't prioritize it now on our side (read our newest roadmap entry for more context), therefore I see two options:

1. We introduce all the necessary changes (for all the data types) and tests while working on the snowflake_external_function resource stabilization. We can't share any timelines at the moment, but for sure after GA.
2. You continue to work on this PR with our guidance and following our contribution guidelines. Here there are two suboptions:
   • you follow the missing parts described in 1.
   • you add the bare minimum needed, which would be: adding acceptance tests only for VARCHAR documenting the error and solution, and checking the behavior with VARCHAR parameters and without them - we will handle all other data types and tests later on.

Please let us know which option would you prefer.

[sfc-gh-asawicki]

What about other types (like number, timestamp, etc.)? They also come in with-parameters and without-parameters flavours and they should be addressed too.

[nijave]

Really imo it seems much easier to just remove all the validation logic and let Snowflake handle it server-side on apply. Snowflake already has validation logic and it seems counterproductive trying to reproduce/reverse engineer that closed source logic in an OSS project.

[sfc-gh-asawicki]

As I mentioned, the data types will be reworked in the provider. This logic will probably be trashed either way.

However, we need to handle the data type logic on the provider side to correctly handle plans and diff suppressions; we can't just remove it and hope for the best; these resources would be unusable.

Additionally, there are users, like you, who would prefer the "applies" to fail (full sf side validation), but there are also those who would like to have almost 100% parity between successful "plans" and successful "applies." The current approach lies somewhere in the middle.

[nijave]

However, we need to handle the data type logic on the provider side to correctly handle plans and diff suppressions; we can't just remove it and hope for the best; these resources would be unusable.

I'm not sure what you mean here. Diff suppression is separate and orthogonal to config validation. It is very common practice in providers to do minimal config validation and allow the API to do full validation. Ref: AWS Terraform Provider

but there are also those who would like to have almost 100% parity between successful "plans" and successful "applies."

That's exactly the issue here. VARCHAR(10) plans correctly but fails to apply since VARCHAR(10) is submitted to Snowflake but VARCHAR is returned from Snowflake which fails validation when Terraform tries to save/update state

[sfc-gh-asawicki]

That's exactly the issue here. VARCHAR(10) plans correctly but fails to apply since VARCHAR(10) is submitted to Snowflake but VARCHAR is returned from Snowflake which fails validation when Terraform tries to save/update state

No argument here: we know this preview resource is not handling data types correctly, we did not come up with this logic, and we will correct it while reworking this resource.

I'm not sure what you mean here. Diff suppression is separate and orthogonal to config validation. It is very common practice in providers to do minimal config validation and allow the API to do full validation. Ref: AWS Terraform Provider

Unfortunately, some logic is needed if you want to handle both the user config changes and the external changes properly and, at the same time, allow users to use data types without attributes (using default ones, e.g. NUMBER = NUMBER(38, 0)). For example, you need to be able to differentiate between these situations:

1. User sets VARCHAR(10) in config; then, they change it to VARCHAR (change should happen)
2. User sets VARCHAR(10) in config; Snowflake returns VARCHAR (change should be suppressed)

Other example:

1. User sets NUMBER in the config, Snowflake returns NUMBER(38, 0) (change should be suppressed)
2. User sets NUMBER in the config; then, they change it to NUMBER(38, 0) (change should happen - or not, depending on the approach we take; the question is how do we want to handle the default change in Snowflake and how would it be performed on SNowflake side for the existing objects)
3. User sets NUMBER in the config; later on, Snowflake returns NUMBER(38, 2) (change should happen)

All these situations will be handled correctly when we rework this preview resource, which will not happen any time soon.

[nijave]

I would like to add to what @sfc-gh-jmichalak wrote. We need to validate the previous behavior and the proposed correctness (that's why we require at least acceptance tests). Ideally, we should address all the data types that are potentially affected (I doubt that VARCHAR is the only invalid one here). Because of that, adding an integration test to the DESCRIBE behavior should be added to the tests set (similar to e.g.

terraform-provider-snowflake/pkg/sdk/testint/functions_integration_test.go

Line 1981 in 9be2196

for _, tc := range []string{

). This is a preview resource and we won't prioritize it now on our side (read our newest roadmap entry for more context), therefore I see two options:

1. We introduce all the necessary changes (for all the data types) and tests while working on the snowflake_external_function resource stabilization. We can't share any timelines at the moment, but for sure after GA.

2. You continue to work on this PR with our guidance and following our contribution guidelines. Here there are two suboptions:

   • you follow the missing parts described in 1.
   • you add the bare minimum needed, which would be: adding acceptance tests only for VARCHAR documenting the error and solution, and checking the behavior with VARCHAR parameters and without them - we will handle all other data types and tests later on.

Please let us know which option would you prefer.

#2

Really it probably also needs a diff suppression function and potentially other changes. I don't know very much about datatypes and their intended behavior. The main goal is to be able to use the provider for an external_function with return_type VARCHAR which currently isn't possible (an error is encountered with and without specifying a size)

[sfc-gh-jmichalak]

/ok-to-test sha=c97724548d8e0bdbe3c03814ffc9f6b301cc4811

[github-actions]

Integration tests failure for c97724548d8e0bdbe3c03814ffc9f6b301cc4811

[github-actions]

Integration tests failure for c97724548d8e0bdbe3c03814ffc9f6b301cc4811

[sfc-gh-jmichalak]

@nijave Please merge our dev branch to yours and push. For the future: please don't use force-push. For some reason, the tests are failing now, let's see if it improves after that.

[nijave]

@nijave Please merge our dev branch to yours and push. For the future: please don't use force-push. For some reason, the tests are failing now, let's see if it improves after that.

➜  terraform-provider-snowflake git:(nv-fix-varchar) git fetch --all
Fetching origin
Fetching fork
➜  terraform-provider-snowflake git:(nv-fix-varchar) git merge origin/dev
Already up to date.

I think what's happening is the slash command sends repository_dispatch event which uses repo default branch (main) which you can see since that sha is being built (ref https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#repository_dispatch).

Looks like when using slash commands, you need to override the ref for the actions/checkout. See peter-evans/slash-command-dispatch#141 (it looks like the info is already pulled verify_sha_input step but not passed to checkout https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/.github/workflows/tests.yml#L16

That said, probably easiest work around is to pull my branch and push to Snowflake-Labs/terraform-provider-snowflake and open a new PR

[sfc-gh-jmichalak]

Thanks for the detailed explanation! We had a team discussion about this and we agreed there's a problem with our dispatch pipeline (may be related to our change from main to dev as a base branch). It looks like we need to adjust the dispatch/testing pipeline so it uses the correct refs in both dispatched events and pull requests from the maintainers.

We will address this issue soon because it affects all community PRs. I ran the tests locally, and everything seems fine, so I'm merging this for now. This fix will be included in the next release.

Please remember that this resource will be reworked after GA; this includes potential breaking changes.

[nijave]

thanks guys!