fix: Fix provider config hierarchy #2551

sfc-gh-asawicki · 2024-02-23T14:30:03Z

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

github-actions · 2024-02-23T14:31:35Z

Integration tests failure for c4eec4634c01a46f776d2c54c87d5c66fc6f2478

github-actions · 2024-02-23T15:29:25Z

Integration tests success for 50bbfa78b55d13bc7db795eb88490763638fbd27

sfc-gh-swinkler · 2024-02-24T02:28:11Z

pkg/sdk/config.go

@@ -12,8 +12,8 @@ import (
 func DefaultConfig() *gosnowflake.Config {
 	config, err := ProfileConfig("default")
 	if err != nil || config == nil {
-		log.Printf("[DEBUG] No Snowflake config file found, falling back to environment variables: %v\n", err)
-		return EnvConfig()
+		log.Printf("[DEBUG] No Snowflake config file found, returning empty config: %v\n", err)


i feel like we should be able to read from environment variables, but maybe to avoid conflict, we can slightly rename them? LIke SF_USER instead of SNOWFLAKE_USER?

I agree. But as I stated in the PR description: 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). This will matter after we extract the SDK into its own being. Before that, we try env vars first, so there is no need to do that here, and was in fact the source of errors.

sfc-gh-swinkler · 2024-02-24T02:29:31Z

pkg/sdk/config_test.go

-		assert.Equal(t, "TEST_USER", config.User)
-		assert.Equal(t, "abcd1234", config.Password)
-		assert.Equal(t, "ACCOUNTADMIN", config.Role)
+	t.Run("merge configs reverted", func(t *testing.T) {


i think the word is "inverted" not "reverted"

sfc-gh-swinkler · 2024-02-24T02:31:04Z

pkg/provider/provider_acceptance_test.go

+	})
+}
+
+func emptyProviderConfig() string {


shouldnt we be putting these in a testdata directory?

We did not agree to automatically move every config to a file - it is not necessarily more readable. Currently, we accept both options (they are both not ideal), but we have ideas for the better, third option (maybe coming soon).

sfc-gh-swinkler · 2024-02-24T02:32:08Z

pkg/internal/snowflakeenvs/snowflake_environment_variables.go

@@ -0,0 +1,10 @@
+package snowflakeenvs


should this not be under acceptance package?

acceptance package is used for tests only whereas this one can be used for production code too.

sfc-gh-swinkler · 2024-02-24T02:34:34Z

pkg/acceptance/testenvs/testing_environment_variables.go

+	return env
+}
+
+type Env interface {


is this interface necessary?

It's not necessary but it's fun. It prevents you from calling the GetOrSkipTest with anything else than listed env vars (on compilation level).

sfc-gh-swinkler · 2024-02-24T02:36:17Z

pkg/acceptance/testenvs/testenvs_test.go

+}
+
+func Test_Assertions(t *testing.T) {
+	runAssertionInGoroutineAndWait := func(assertion func()) {


should this be named: runAsyncWithDelay()?

No. I think the current name is better. We don't want delay, we want the goroutine to complete. I can change it to runAssertionInGoroutineAndWaitForCompletion.

sfc-gh-swinkler · 2024-02-24T02:37:16Z

pkg/acceptance/testenvs/testenvs_test.go

+)
+
+func Test_GetOrSkipTest(t *testing.T) {
+	runGetOrSkipInGoroutineAndWait := func(t *testing.T) string {


you also have this defined below. should this be refactored out into a helper function

There are differences between them (e.g. returning value). To make it more general we should support variadic return values and I don't think this is needed now.

pkg/acceptance/testenvs/testenvs_test.go

github-actions · 2024-02-26T12:31:40Z

Integration tests failure for a2e4994ee909f71592e9b8a29ab85a97ec99f700

🤖 I have created a release *beep* *boop* --- ## [0.87.0](v0.86.0...v0.87.0) (2024-02-28) ### 🎉 **What's new:** * Add network rule to the sdk ([#2526](#2526)) ([b379565](b379565)) * supports collation of table column ([#2496](#2496)) ([56771f1](56771f1)) ### 🔧 **Misc** * Clean up environment variables in tests and on CI ([#2543](#2543)) ([9a10cb1](9a10cb1)) * replace warning in new grant resources with info log ([#2521](#2521)) ([c3014b9](c3014b9)) ### 🐛 **Bug fixes:** * data retention days follow up ([#2566](#2566)) ([7aba384](7aba384)) * data retention time parameters ([#2502](#2502)) ([76abf21](76abf21)) * data retention time parameters follow-up ([#2530](#2530)) ([5544544](5544544)) * Demote warning to info and set volatility for procedures and functions ([#2567](#2567)) ([abaad7c](abaad7c)), closes [#2536](#2536) * Fix ACCOUNT PARAMETERS option failover group resource ([#2522](#2522)) ([61883f3](61883f3)), closes [#2517](#2517) * Fix failover group alter syntax and suppression for pipe statement ([#2562](#2562)) ([24d76c3](24d76c3)) * Fix few tests ([#2515](#2515)) ([a523a6b](a523a6b)) * Fix provider config hierarchy ([#2551](#2551)) ([677a12b](677a12b)) * Fix query_results in unsafe_execute resource ([#2512](#2512)) ([94ca158](94ca158)), closes [#2491](#2491) * Fix replication for database resource ([#2524](#2524)) ([767fbce](767fbce)), closes [#2021](#2021) * Fix show by id for external functions ([#2531](#2531)) ([d910a84](d910a84)), closes [#2528](#2528) * Fix various small problems ([#2552](#2552)) ([f558ce6](f558ce6)) * Granting database roles ([#2511](#2511)) ([dc27d64](dc27d64)), closes [#2402](#2402) * grants on external volumes ([#2538](#2538)) ([1de9a29](1de9a29)) * Handle old reference for table_id in table constraint resource ([#2558](#2558)) ([d1e8912](d1e8912)), closes [#2535](#2535) * loosen identifier field validation for account object identifiers ([#2564](#2564)) ([a5ed8cd](a5ed8cd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>

sfc-gh-asawicki added 12 commits February 23, 2024 12:34

Add test proving #2294 and #2242

cf186e0

Fix the implementation

7b9a30d

Introduce constants and use them in provider acceptance test

13d0b10

Add test convenience method

7b92757

Limit invocations to our predefined envs only

40a72e2

Fix tests

a273d2f

Replace string usages of secondary profile

f68b994

Add merge config tests

d2727c5

Test get or skip

7078688

Test assertions

a1e0409

Run linters

2c3bb13

Add note to the migration guide

c4eec46

Trigger build

50bbfa7

sfc-gh-asawicki mentioned this pull request Feb 23, 2024

[Ubuntu] Error creating snowflake_procedure, snowflake_user and snowflake_warehouse ressources #2542

Closed

sfc-gh-asawicki marked this pull request as ready for review February 23, 2024 15:47

sfc-gh-asawicki requested review from sfc-gh-jcieslak and sfc-gh-swinkler February 23, 2024 15:47

sfc-gh-swinkler reviewed Feb 24, 2024

View reviewed changes

sfc-gh-jcieslak reviewed Feb 26, 2024

View reviewed changes

pkg/acceptance/testenvs/testenvs_test.go Outdated Show resolved Hide resolved

pkg/acceptance/testenvs/testenvs_test.go Outdated Show resolved Hide resolved

sfc-gh-asawicki added 2 commits February 26, 2024 12:22

Merge branch 'main' into 2294-2242-fix-provider-config-hierarchy

50772f8

Fix after reviews

a2e4994

sfc-gh-asawicki requested a review from sfc-gh-swinkler February 26, 2024 11:53

sfc-gh-jcieslak approved these changes Feb 26, 2024

View reviewed changes

sfc-gh-asawicki merged commit 677a12b into main Feb 26, 2024
8 of 9 checks passed

sfc-gh-asawicki deleted the 2294-2242-fix-provider-config-hierarchy branch February 26, 2024 12:50

snowflake-release-please bot mentioned this pull request Feb 26, 2024

chore(main): release 0.87.0 #2513

Merged

sfc-gh-asawicki mentioned this pull request Feb 27, 2024

chore: add username_password auth test #2184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix provider config hierarchy #2551

fix: Fix provider config hierarchy #2551

sfc-gh-asawicki commented Feb 23, 2024 •

edited

Loading

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024 •

edited

Loading

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024

sfc-gh-asawicki Feb 26, 2024

sfc-gh-swinkler Feb 24, 2024

sfc-gh-asawicki Feb 26, 2024 •

edited

Loading

github-actions bot commented Feb 26, 2024

fix: Fix provider config hierarchy #2551

fix: Fix provider config hierarchy #2551

Conversation

sfc-gh-asawicki commented Feb 23, 2024 • edited Loading

github-actions bot commented Feb 23, 2024

github-actions bot commented Feb 23, 2024

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2024

sfc-gh-asawicki commented Feb 23, 2024 •

edited

Loading

sfc-gh-asawicki Feb 26, 2024 •

edited

Loading

sfc-gh-asawicki Feb 26, 2024 •

edited

Loading