-
Notifications
You must be signed in to change notification settings - Fork 427
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
incorrect order of precedence for SNOWFLAKE_ACCOUNT variable #2294
Comments
Hey @coleheflin-gt. Thanks for reporting the issue. This is still a blowback after unfortunate #2126. BTW, the documentation needed to be corrected before (now and before the refactor documentation was wrong). I will talk with the team. We have to decide:
I will get back to you as soon as we have any conclusions. |
@sfc-gh-asawicki Thanks for the quick response, that makes sense! My preference is to maintain the existing order of precedence outlined in the documentation. When we explicitly pass a value into a provider it should take precedence otherwise it will lead to confusion of where that value is being set. In our scenario, the production snowflake account value was set as an environment variable (for a different repository not related to snowflake terraform work) and I was on the terraform staging workspace expecting to be making changes in staging (since the staging account is passed into the provider explicitly) but I was running tf plans in production instead. Luckily the build was broken due to some changes so I never actually applied changes in production (when I thought I was in staging). To minimize breaking changes, perhaps your team could implement logging when a plan/apply occurs that displays which account it is set to and where it got it from. I realize you have to take into account many team's use cases, but I hope the described situation is helpful in your team coming to a decision! Thanks for the support on this. |
It actually impacts all environment variables, not only |
Hey @AZenat @coleheflin-gt. We will work on this issue this week. I hope it will land with the next week's release. I will keep you posted. |
Before fixing #2294 and #2242 we need to have a clean CI env. After this PR it should be possible to completely remove following environment variables from the secrets: - AWS_EXTERNAL_BUCKET_URL (it has TEST_SF_TF_AWS_EXTERNAL_BUCKET_URL replacement) - AWS_EXTERNAL_KEY_ID (it has TEST_SF_TF_AWS_EXTERNAL_KEY_ID replacement) - AWS_EXTERNAL_ROLE_ARN (it has TEST_SF_TF_AWS_EXTERNAL_ROLE_ARN replacement) - AWS_EXTERNAL_SECRET_KEY (it has TEST_SF_TF_AWS_EXTERNAL_SECRET_KEY replacement) - AZURE_EXTERNAL_BUCKET_URL (it has TEST_SF_TF_AZURE_EXTERNAL_BUCKET_URL replacement) - AZURE_EXTERNAL_SAS_TOKEN (it has TEST_SF_TF_AZURE_EXTERNAL_SAS_TOKEN replacement) - AZURE_EXTERNAL_TENANT_ID (it has TEST_SF_TF_AZURE_EXTERNAL_TENANT_ID replacement) - GCS_EXTERNAL_BUCKET_URL (it has TEST_SF_TF_GCS_EXTERNAL_BUCKET_URL replacement) - SKIP_EMAIL_INTEGRATION_TESTS (not used) - SKIP_EXTERNAL_TABLE_TEST (not used anymore) - SKIP_NOTIFICATION_INTEGRATION_TESTS (not used) - SKIP_STREAM_TEST (not used anymore) - SNOWFLAKE_ACCOUNT (we use the config now, this shouldn't be needed) - SNOWFLAKE_ACCOUNT_SECOND (not used anymore) - SNOWFLAKE_ACCOUNT_THIRD (not used anymore) - SNOWFLAKE_PASSWORD (we use the config now, this shouldn't be needed) - SNOWFLAKE_ROLE (we use the config now, this shouldn't be needed) - SNOWFLAKE_USER (we use the config now, this shouldn't be needed) There are three more that should be removed but they should be approached more carefully: - SNOWFLAKE_WAREHOUSE (we do not have it in our config, so it is taken from env; test what happens without setting a warehouse by running the tests locally with the config similar to the CI one) - SNOWFLAKE_PORT (we do not have this in our config, let's check if the tests will work without this - they should not fail) - SNOWFLAKE_PROTOCOL (we do not have this in our config, there is a default, it should be fine without it)
Fix the order of precedence for the provider's config: - removed fallback to environment variables for the SDK client (we will address the setup of the client separately with the extraction of the SDK client from this repository) - fixed the merge config function to fill out only missing parameters - added missing tests - tested the provider setup in an acceptance test - introduced environment helpers with tests - started to move env to one place (the rest envs can be moved after discussing in this PR if we all agree with that direction) - added migration notes References: #2242 #2294
Hey @AZenat @coleheflin-gt. We have released the fix as part of v0.87.0 release. Please follow the migration guide during the update. Please confirm that the issue is resolved in the newest version. Thanks! |
@sfc-gh-asawicki Thanks for the update. We upgraded to the 0.87.0 and it works as expected! The issue has been resolved. Thanks for the work on this. |
Great! I'm closing the issue then. |
Terraform CLI and Provider Versions
Terraform v1.6.1
Snowflake Labs 0.80.0
Terraform Configuration
Expected Behavior
Based on the terraform docs (scroll to the bottom), "The Snowflake provider will use the following order of precedence when determining which credentials to use: 1) Provider Configuration 2) Environment Variables 3) Config File".
We are expecting the local.snowflake_account variable to be used by the snowflake provider.
Actual Behavior
We are passing in the snowflake account directly into the provider and we also have an environment variable called SNOWFLAKE_ACCOUNT. The account passed directly into the provider is being overwritten and the value in the SNOWFLAKE_ACCOUNT environment variable is being used.
Steps to Reproduce
How much impact is this issue causing?
High
Logs
No response
Additional Information
No response
The text was updated successfully, but these errors were encountered: