From cf186e078e27d402b773c7b9796269f2e15a2d88 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 12:18:15 +0100 Subject: [PATCH 01/14] Add test proving #2294 and #2242 --- pkg/provider/provider_acceptance_test.go | 136 +++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 pkg/provider/provider_acceptance_test.go diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go new file mode 100644 index 0000000000..db0eddbe01 --- /dev/null +++ b/pkg/provider/provider_acceptance_test.go @@ -0,0 +1,136 @@ +package provider_test + +import ( + "fmt" + "github.com/stretchr/testify/require" + "os" + "regexp" + "testing" + + acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/tfversion" +) + +func TestAcc_Provider_configHierarchy(t *testing.T) { + user := os.Getenv("TEST_SF_TF_USER") + pass := os.Getenv("TEST_SF_TF_PASSWORD") + account := os.Getenv("TEST_SF_TF_ACCOUNT") + role := os.Getenv("TEST_SF_TF_ROLE") + host := os.Getenv("TEST_SF_TF_HOST") + if user == "" || pass == "" || account == "" || role == "" || host == "" { + t.Skip("Skipping TestAcc_Provider_configHierarchy") + } + + nonExistingUser := "non-existing-user" + profileWithIncorrectUserAndPassword := "incorrect_test_profile" + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + Steps: []resource.TestStep{ + // make sure that we fail for incorrect profile + { + Config: providerConfig(profileWithIncorrectUserAndPassword), + ExpectError: regexp.MustCompile("Incorrect username or password was specified"), + }, + // incorrect user in provider config should not be rewritten by profile and cause error + { + Config: providerConfigWithUser(nonExistingUser, "default"), + ExpectError: regexp.MustCompile("Incorrect username or password was specified"), + }, + // correct user and password in provider's config should not be rewritten by a faulty config + { + Config: providerConfigWithUserAndPassword(user, pass, profileWithIncorrectUserAndPassword), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName), + ), + }, + // incorrect user in env variable should not be rewritten by profile and cause error + { + PreConfig: func() { + t.Setenv("SNOWFLAKE_USER", nonExistingUser) + }, + Config: providerConfig("default"), + ExpectError: regexp.MustCompile("Incorrect username or password was specified"), + }, + // correct user and password in env should not be rewritten by a faulty config + { + PreConfig: func() { + t.Setenv("SNOWFLAKE_USER", user) + t.Setenv("SNOWFLAKE_PASSWORD", pass) + }, + Config: providerConfig(profileWithIncorrectUserAndPassword), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName), + ), + }, + // user on provider level wins (it's incorrect - env and profile ones are) + { + Config: providerConfigWithUser(nonExistingUser, "default"), + ExpectError: regexp.MustCompile("Incorrect username or password was specified"), + }, + // there is no config (by setting the dir to something different than .snowflake/config) + { + PreConfig: func() { + dir, err := os.UserHomeDir() + require.NoError(t, err) + t.Setenv("SNOWFLAKE_CONFIG_PATH", dir) + }, + Config: providerConfigWithUserAndPassword(user, pass, "default"), + ExpectError: regexp.MustCompile("account is empty"), + }, + // provider's config should not be rewritten by env when there is no profile (incorrect user in config versus correct one in env) - proves #2242 + { + PreConfig: func() { + require.NotEmpty(t, os.Getenv("SNOWFLAKE_CONFIG_PATH")) + t.Setenv("SNOWFLAKE_USER", user) + t.Setenv("SNOWFLAKE_PASSWORD", pass) + t.Setenv("SNOWFLAKE_ACCOUNT", account) + t.Setenv("SNOWFLAKE_ROLE", role) + t.Setenv("SNOWFLAKE_HOST", host) + }, + Config: providerConfigWithUser(nonExistingUser, "default"), + ExpectError: regexp.MustCompile("Incorrect username or password was specified"), + }, + }, + }) +} + +func providerConfig(profile string) string { + return fmt.Sprintf(` +provider "snowflake" { + profile = "%[1]s" +} +`, profile) + datasourceConfig() +} + +func providerConfigWithUser(user string, profile string) string { + return fmt.Sprintf(` +provider "snowflake" { + user = "%[1]s" + profile = "%[2]s" +} +`, user, profile) + datasourceConfig() +} + +func providerConfigWithUserAndPassword(user string, pass string, profile string) string { + return fmt.Sprintf(` +provider "snowflake" { + user = "%[1]s" + password = "%[2]s" + profile = "%[3]s" +} +`, user, pass, profile) + datasourceConfig() +} + +func datasourceConfig() string { + return fmt.Sprintf(` +data snowflake_database "t" { + name = "%s" +}`, acc.TestDatabaseName) +} From 7b9a30d04ae904d9a11d8f0600494c4ee734fcf9 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 12:44:28 +0100 Subject: [PATCH 02/14] Fix the implementation --- pkg/provider/provider_acceptance_test.go | 21 +++++++++++++++++++++ pkg/sdk/config.go | 16 ++++++++-------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index db0eddbe01..ad00b1d064 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -97,10 +97,31 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { Config: providerConfigWithUser(nonExistingUser, "default"), ExpectError: regexp.MustCompile("Incorrect username or password was specified"), }, + // make sure the teardown is fine by using a correct env config at the end + { + PreConfig: func() { + require.NotEmpty(t, os.Getenv("SNOWFLAKE_CONFIG_PATH")) + require.NotEmpty(t, os.Getenv("SNOWFLAKE_USER")) + require.NotEmpty(t, os.Getenv("SNOWFLAKE_PASSWORD")) + require.NotEmpty(t, os.Getenv("SNOWFLAKE_ACCOUNT")) + require.NotEmpty(t, os.Getenv("SNOWFLAKE_ROLE")) + require.NotEmpty(t, os.Getenv("SNOWFLAKE_HOST")) + }, + Config: emptyProviderConfig(), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName), + ), + }, }, }) } +func emptyProviderConfig() string { + return ` +provider "snowflake" { +}` + datasourceConfig() +} + func providerConfig(profile string) string { return fmt.Sprintf(` provider "snowflake" { diff --git a/pkg/sdk/config.go b/pkg/sdk/config.go index b30cb85acb..ec85886c00 100644 --- a/pkg/sdk/config.go +++ b/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) + config = &gosnowflake.Config{} } return config } @@ -51,22 +51,22 @@ func MergeConfig(baseConfig *gosnowflake.Config, mergeConfig *gosnowflake.Config if baseConfig == nil { return mergeConfig } - if mergeConfig.Account != "" { + if baseConfig.Account == "" { baseConfig.Account = mergeConfig.Account } - if mergeConfig.User != "" { + if baseConfig.User == "" { baseConfig.User = mergeConfig.User } - if mergeConfig.Password != "" { + if baseConfig.Password == "" { baseConfig.Password = mergeConfig.Password } - if mergeConfig.Role != "" { + if baseConfig.Role == "" { baseConfig.Role = mergeConfig.Role } - if mergeConfig.Region != "" { + if baseConfig.Region == "" { baseConfig.Region = mergeConfig.Region } - if mergeConfig.Host != "" { + if baseConfig.Host == "" { baseConfig.Host = mergeConfig.Host } return baseConfig From 13d0b10a6ced34ab9d4b4b1362d5f2f312329e33 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 13:20:31 +0100 Subject: [PATCH 03/14] Introduce constants and use them in provider acceptance test --- .../testenvs/testing_environment_variables.go | 7 +++ .../testprofiles/testing_config_profiles.go | 5 ++ .../snowflake_environment_variables.go | 8 +++ pkg/provider/provider_acceptance_test.go | 62 ++++++++++--------- 4 files changed, 52 insertions(+), 30 deletions(-) create mode 100644 pkg/acceptance/testenvs/testing_environment_variables.go create mode 100644 pkg/acceptance/testprofiles/testing_config_profiles.go create mode 100644 pkg/internal/snowflakeenvs/snowflake_environment_variables.go diff --git a/pkg/acceptance/testenvs/testing_environment_variables.go b/pkg/acceptance/testenvs/testing_environment_variables.go new file mode 100644 index 0000000000..19f925e150 --- /dev/null +++ b/pkg/acceptance/testenvs/testing_environment_variables.go @@ -0,0 +1,7 @@ +package testenvs + +const User = "TEST_SF_TF_USER" +const Password = "TEST_SF_TF_PASSWORD" +const Account = "TEST_SF_TF_ACCOUNT" +const Role = "TEST_SF_TF_ROLE" +const Host = "TEST_SF_TF_HOST" diff --git a/pkg/acceptance/testprofiles/testing_config_profiles.go b/pkg/acceptance/testprofiles/testing_config_profiles.go new file mode 100644 index 0000000000..b9b9b90e8a --- /dev/null +++ b/pkg/acceptance/testprofiles/testing_config_profiles.go @@ -0,0 +1,5 @@ +package testprofiles + +const Default = "default" +const Secondary = "secondary_test_account" +const IncorrectUserAndPassword = "incorrect_test_profile" diff --git a/pkg/internal/snowflakeenvs/snowflake_environment_variables.go b/pkg/internal/snowflakeenvs/snowflake_environment_variables.go new file mode 100644 index 0000000000..916d4f4898 --- /dev/null +++ b/pkg/internal/snowflakeenvs/snowflake_environment_variables.go @@ -0,0 +1,8 @@ +package snowflakeenvs + +const Account = "SNOWFLAKE_ACCOUNT" +const User = "SNOWFLAKE_USER" +const Password = "SNOWFLAKE_PASSWORD" +const Role = "SNOWFLAKE_ROLE" +const ConfigPath = "SNOWFLAKE_CONFIG_PATH" +const Host = "SNOWFLAKE_HOST" diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index ad00b1d064..11d439a935 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -9,22 +9,24 @@ import ( acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) func TestAcc_Provider_configHierarchy(t *testing.T) { - user := os.Getenv("TEST_SF_TF_USER") - pass := os.Getenv("TEST_SF_TF_PASSWORD") - account := os.Getenv("TEST_SF_TF_ACCOUNT") - role := os.Getenv("TEST_SF_TF_ROLE") - host := os.Getenv("TEST_SF_TF_HOST") + user := os.Getenv(testenvs.User) + pass := os.Getenv(testenvs.Password) + account := os.Getenv(testenvs.Account) + role := os.Getenv(testenvs.Role) + host := os.Getenv(testenvs.Host) if user == "" || pass == "" || account == "" || role == "" || host == "" { t.Skip("Skipping TestAcc_Provider_configHierarchy") } nonExistingUser := "non-existing-user" - profileWithIncorrectUserAndPassword := "incorrect_test_profile" resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, @@ -35,17 +37,17 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { Steps: []resource.TestStep{ // make sure that we fail for incorrect profile { - Config: providerConfig(profileWithIncorrectUserAndPassword), + Config: providerConfig(testprofiles.IncorrectUserAndPassword), ExpectError: regexp.MustCompile("Incorrect username or password was specified"), }, // incorrect user in provider config should not be rewritten by profile and cause error { - Config: providerConfigWithUser(nonExistingUser, "default"), + Config: providerConfigWithUser(nonExistingUser, testprofiles.Default), ExpectError: regexp.MustCompile("Incorrect username or password was specified"), }, // correct user and password in provider's config should not be rewritten by a faulty config { - Config: providerConfigWithUserAndPassword(user, pass, profileWithIncorrectUserAndPassword), + Config: providerConfigWithUserAndPassword(user, pass, testprofiles.IncorrectUserAndPassword), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName), ), @@ -53,25 +55,25 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { // incorrect user in env variable should not be rewritten by profile and cause error { PreConfig: func() { - t.Setenv("SNOWFLAKE_USER", nonExistingUser) + t.Setenv(snowflakeenvs.User, nonExistingUser) }, - Config: providerConfig("default"), + Config: providerConfig(testprofiles.Default), ExpectError: regexp.MustCompile("Incorrect username or password was specified"), }, // correct user and password in env should not be rewritten by a faulty config { PreConfig: func() { - t.Setenv("SNOWFLAKE_USER", user) - t.Setenv("SNOWFLAKE_PASSWORD", pass) + t.Setenv(snowflakeenvs.User, user) + t.Setenv(snowflakeenvs.Password, pass) }, - Config: providerConfig(profileWithIncorrectUserAndPassword), + Config: providerConfig(testprofiles.IncorrectUserAndPassword), Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr("data.snowflake_database.t", "name", acc.TestDatabaseName), ), }, // user on provider level wins (it's incorrect - env and profile ones are) { - Config: providerConfigWithUser(nonExistingUser, "default"), + Config: providerConfigWithUser(nonExistingUser, testprofiles.Default), ExpectError: regexp.MustCompile("Incorrect username or password was specified"), }, // there is no config (by setting the dir to something different than .snowflake/config) @@ -79,33 +81,33 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { PreConfig: func() { dir, err := os.UserHomeDir() require.NoError(t, err) - t.Setenv("SNOWFLAKE_CONFIG_PATH", dir) + t.Setenv(snowflakeenvs.ConfigPath, dir) }, - Config: providerConfigWithUserAndPassword(user, pass, "default"), + Config: providerConfigWithUserAndPassword(user, pass, testprofiles.Default), ExpectError: regexp.MustCompile("account is empty"), }, // provider's config should not be rewritten by env when there is no profile (incorrect user in config versus correct one in env) - proves #2242 { PreConfig: func() { - require.NotEmpty(t, os.Getenv("SNOWFLAKE_CONFIG_PATH")) - t.Setenv("SNOWFLAKE_USER", user) - t.Setenv("SNOWFLAKE_PASSWORD", pass) - t.Setenv("SNOWFLAKE_ACCOUNT", account) - t.Setenv("SNOWFLAKE_ROLE", role) - t.Setenv("SNOWFLAKE_HOST", host) + require.NotEmpty(t, os.Getenv(snowflakeenvs.ConfigPath)) + t.Setenv(snowflakeenvs.User, user) + t.Setenv(snowflakeenvs.Password, pass) + t.Setenv(snowflakeenvs.Account, account) + t.Setenv(snowflakeenvs.Role, role) + t.Setenv(snowflakeenvs.Host, host) }, - Config: providerConfigWithUser(nonExistingUser, "default"), + Config: providerConfigWithUser(nonExistingUser, testprofiles.Default), ExpectError: regexp.MustCompile("Incorrect username or password was specified"), }, // make sure the teardown is fine by using a correct env config at the end { PreConfig: func() { - require.NotEmpty(t, os.Getenv("SNOWFLAKE_CONFIG_PATH")) - require.NotEmpty(t, os.Getenv("SNOWFLAKE_USER")) - require.NotEmpty(t, os.Getenv("SNOWFLAKE_PASSWORD")) - require.NotEmpty(t, os.Getenv("SNOWFLAKE_ACCOUNT")) - require.NotEmpty(t, os.Getenv("SNOWFLAKE_ROLE")) - require.NotEmpty(t, os.Getenv("SNOWFLAKE_HOST")) + require.NotEmpty(t, os.Getenv(snowflakeenvs.ConfigPath)) + require.NotEmpty(t, os.Getenv(snowflakeenvs.User)) + require.NotEmpty(t, os.Getenv(snowflakeenvs.Password)) + require.NotEmpty(t, os.Getenv(snowflakeenvs.Account)) + require.NotEmpty(t, os.Getenv(snowflakeenvs.Role)) + require.NotEmpty(t, os.Getenv(snowflakeenvs.Host)) }, Config: emptyProviderConfig(), Check: resource.ComposeTestCheckFunc( From 7b92757cddca379da535ec1a52523a7b9c2893c4 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 13:30:23 +0100 Subject: [PATCH 04/14] Add test convenience method --- .../testenvs/testing_environment_variables.go | 15 +++++++++++++++ pkg/provider/provider_acceptance_test.go | 13 +++++-------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/pkg/acceptance/testenvs/testing_environment_variables.go b/pkg/acceptance/testenvs/testing_environment_variables.go index 19f925e150..4f943a60eb 100644 --- a/pkg/acceptance/testenvs/testing_environment_variables.go +++ b/pkg/acceptance/testenvs/testing_environment_variables.go @@ -1,7 +1,22 @@ package testenvs +import ( + "os" + "testing" +) + const User = "TEST_SF_TF_USER" const Password = "TEST_SF_TF_PASSWORD" const Account = "TEST_SF_TF_ACCOUNT" const Role = "TEST_SF_TF_ROLE" const Host = "TEST_SF_TF_HOST" + +// TODO: allow to be used only with the set above +// TODO: test +func GetOrSkipTest(t *testing.T, envName string) string { + env := os.Getenv(envName) + if env == "" { + t.Skipf("Skipping %s", t.Name()) + } + return env +} diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index 11d439a935..5b4a35aae7 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -17,14 +17,11 @@ import ( ) func TestAcc_Provider_configHierarchy(t *testing.T) { - user := os.Getenv(testenvs.User) - pass := os.Getenv(testenvs.Password) - account := os.Getenv(testenvs.Account) - role := os.Getenv(testenvs.Role) - host := os.Getenv(testenvs.Host) - if user == "" || pass == "" || account == "" || role == "" || host == "" { - t.Skip("Skipping TestAcc_Provider_configHierarchy") - } + user := testenvs.GetOrSkipTest(t, testenvs.User) + pass := testenvs.GetOrSkipTest(t, testenvs.Password) + account := testenvs.GetOrSkipTest(t, testenvs.Account) + role := testenvs.GetOrSkipTest(t, testenvs.Role) + host := testenvs.GetOrSkipTest(t, testenvs.Host) nonExistingUser := "non-existing-user" From 40a72e291d2db2408c92786cd960ba9a90f6b3ff Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 13:37:22 +0100 Subject: [PATCH 05/14] Limit invocations to our predefined envs only --- .../testenvs/testing_environment_variables.go | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/pkg/acceptance/testenvs/testing_environment_variables.go b/pkg/acceptance/testenvs/testing_environment_variables.go index 4f943a60eb..3123e033dc 100644 --- a/pkg/acceptance/testenvs/testing_environment_variables.go +++ b/pkg/acceptance/testenvs/testing_environment_variables.go @@ -1,22 +1,32 @@ package testenvs import ( + "fmt" "os" "testing" ) -const User = "TEST_SF_TF_USER" -const Password = "TEST_SF_TF_PASSWORD" -const Account = "TEST_SF_TF_ACCOUNT" -const Role = "TEST_SF_TF_ROLE" -const Host = "TEST_SF_TF_HOST" +type env string + +const ( + User env = "TEST_SF_TF_USER" + Password env = "TEST_SF_TF_PASSWORD" + Account env = "TEST_SF_TF_ACCOUNT" + Role env = "TEST_SF_TF_ROLE" + Host env = "TEST_SF_TF_HOST" +) -// TODO: allow to be used only with the set above // TODO: test -func GetOrSkipTest(t *testing.T, envName string) string { - env := os.Getenv(envName) +func GetOrSkipTest(t *testing.T, envName Env) string { + env := os.Getenv(fmt.Sprintf("%v", envName)) if env == "" { t.Skipf("Skipping %s", t.Name()) } return env } + +type Env interface { + xxxProtected() +} + +func (e env) xxxProtected() {} From a273d2fdfd6992deb22c8e54c05810b49714801c Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 14:09:30 +0100 Subject: [PATCH 06/14] Fix tests --- pkg/acceptance/testenvs/helpers.go | 16 +++++ .../testenvs/testing_environment_variables.go | 2 +- pkg/provider/provider_acceptance_test.go | 20 +++--- pkg/sdk/client_integration_test.go | 39 ++++++++++-- pkg/sdk/config.go | 28 --------- pkg/sdk/config_test.go | 62 +++++-------------- 6 files changed, 76 insertions(+), 91 deletions(-) create mode 100644 pkg/acceptance/testenvs/helpers.go diff --git a/pkg/acceptance/testenvs/helpers.go b/pkg/acceptance/testenvs/helpers.go new file mode 100644 index 0000000000..9ee7333b07 --- /dev/null +++ b/pkg/acceptance/testenvs/helpers.go @@ -0,0 +1,16 @@ +package testenvs + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func AssertEnvNotSet(t *testing.T, envName string) { + require.Empty(t, os.Getenv(envName)) +} + +func AssertEnvSet(t *testing.T, envName string) { + require.NotEmpty(t, os.Getenv(envName)) +} diff --git a/pkg/acceptance/testenvs/testing_environment_variables.go b/pkg/acceptance/testenvs/testing_environment_variables.go index 3123e033dc..2e3f3d6bee 100644 --- a/pkg/acceptance/testenvs/testing_environment_variables.go +++ b/pkg/acceptance/testenvs/testing_environment_variables.go @@ -20,7 +20,7 @@ const ( func GetOrSkipTest(t *testing.T, envName Env) string { env := os.Getenv(fmt.Sprintf("%v", envName)) if env == "" { - t.Skipf("Skipping %s", t.Name()) + t.Skipf("Skipping %s, env %v missing", t.Name(), envName) } return env } diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index 5b4a35aae7..214ebda1e7 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -27,7 +27,11 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, - PreCheck: func() { acc.TestAccPreCheck(t) }, + PreCheck: func() { + acc.TestAccPreCheck(t) + testenvs.AssertEnvNotSet(t, snowflakeenvs.User) + testenvs.AssertEnvNotSet(t, snowflakeenvs.Password) + }, TerraformVersionChecks: []tfversion.TerraformVersionCheck{ tfversion.RequireAbove(tfversion.Version1_5_0), }, @@ -86,7 +90,7 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { // provider's config should not be rewritten by env when there is no profile (incorrect user in config versus correct one in env) - proves #2242 { PreConfig: func() { - require.NotEmpty(t, os.Getenv(snowflakeenvs.ConfigPath)) + testenvs.AssertEnvSet(t, snowflakeenvs.ConfigPath) t.Setenv(snowflakeenvs.User, user) t.Setenv(snowflakeenvs.Password, pass) t.Setenv(snowflakeenvs.Account, account) @@ -99,12 +103,12 @@ func TestAcc_Provider_configHierarchy(t *testing.T) { // make sure the teardown is fine by using a correct env config at the end { PreConfig: func() { - require.NotEmpty(t, os.Getenv(snowflakeenvs.ConfigPath)) - require.NotEmpty(t, os.Getenv(snowflakeenvs.User)) - require.NotEmpty(t, os.Getenv(snowflakeenvs.Password)) - require.NotEmpty(t, os.Getenv(snowflakeenvs.Account)) - require.NotEmpty(t, os.Getenv(snowflakeenvs.Role)) - require.NotEmpty(t, os.Getenv(snowflakeenvs.Host)) + testenvs.AssertEnvSet(t, snowflakeenvs.ConfigPath) + testenvs.AssertEnvSet(t, snowflakeenvs.User) + testenvs.AssertEnvSet(t, snowflakeenvs.Password) + testenvs.AssertEnvSet(t, snowflakeenvs.Account) + testenvs.AssertEnvSet(t, snowflakeenvs.Role) + testenvs.AssertEnvSet(t, snowflakeenvs.Host) }, Config: emptyProviderConfig(), Check: resource.ComposeTestCheckFunc( diff --git a/pkg/sdk/client_integration_test.go b/pkg/sdk/client_integration_test.go index fdc1ce666d..84d5c17005 100644 --- a/pkg/sdk/client_integration_test.go +++ b/pkg/sdk/client_integration_test.go @@ -3,9 +3,12 @@ package sdk import ( "context" "database/sql" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "os" "testing" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" "github.com/snowflakedb/gosnowflake" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -18,12 +21,36 @@ func TestClient_NewClient(t *testing.T) { require.NoError(t, err) }) - t.Run("uses env vars if values are missing", func(t *testing.T) { - cleanupEnvVars := setupEnvVars(t, "TEST_ACCOUNT", "TEST_USER", "abcd1234", "ACCOUNTADMIN", "") - t.Cleanup(cleanupEnvVars) - config := EnvConfig() - _, err := NewClient(config) - require.Error(t, err) + t.Run("with missing config", func(t *testing.T) { + dir, err := os.UserHomeDir() + require.NoError(t, err) + t.Setenv(snowflakeenvs.ConfigPath, dir) + + config := DefaultConfig() + _, err = NewClient(config) + require.ErrorContains(t, err, "260000: account is empty") + }) + + t.Run("with incorrect config", func(t *testing.T) { + config, err := ProfileConfig(testprofiles.IncorrectUserAndPassword) + require.NoError(t, err) + require.NotNil(t, config) + + _, err = NewClient(config) + require.ErrorContains(t, err, "Incorrect username or password was specified") + }) + + t.Run("with missing config - should not care about correct env variables", func(t *testing.T) { + account := testenvs.GetOrSkipTest(t, testenvs.Account) + t.Setenv(snowflakeenvs.Account, account) + + dir, err := os.UserHomeDir() + require.NoError(t, err) + t.Setenv(snowflakeenvs.ConfigPath, dir) + + config := DefaultConfig() + _, err = NewClient(config) + require.ErrorContains(t, err, "260000: account is empty") }) t.Run("registers snowflake-instrumented driver", func(t *testing.T) { diff --git a/pkg/sdk/config.go b/pkg/sdk/config.go index ec85886c00..8c4441c330 100644 --- a/pkg/sdk/config.go +++ b/pkg/sdk/config.go @@ -87,34 +87,6 @@ func configFile() (string, error) { return filepath.Join(dir, ".snowflake", "config"), nil } -func EnvConfig() *gosnowflake.Config { - config := &gosnowflake.Config{} - - if account, ok := os.LookupEnv("SNOWFLAKE_ACCOUNT"); ok { - config.Account = account - } - if user, ok := os.LookupEnv("SNOWFLAKE_USER"); ok { - config.User = user - } - if password, ok := os.LookupEnv("SNOWFLAKE_PASSWORD"); ok { - config.Password = password - } - if role, ok := os.LookupEnv("SNOWFLAKE_ROLE"); ok { - config.Role = role - } - if region, ok := os.LookupEnv("SNOWFLAKE_REGION"); ok { - config.Region = region - } - if host, ok := os.LookupEnv("SNOWFLAKE_HOST"); ok { - config.Host = host - } - if warehouse, ok := os.LookupEnv("SNOWFLAKE_WAREHOUSE"); ok { - config.Warehouse = warehouse - } - - return config -} - func loadConfigFile() (map[string]*gosnowflake.Config, error) { path, err := configFile() if err != nil { diff --git a/pkg/sdk/config_test.go b/pkg/sdk/config_test.go index 71a33f5533..8e26b6a9dd 100644 --- a/pkg/sdk/config_test.go +++ b/pkg/sdk/config_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -24,8 +25,8 @@ func TestLoadConfigFile(t *testing.T) { role='SECURITYADMIN' ` configPath := testFile(t, "config", []byte(c)) - cleanupEnvVars := setupEnvVars(t, "", "", "", "", configPath) - t.Cleanup(cleanupEnvVars) + t.Setenv(snowflakeenvs.ConfigPath, configPath) + m, err := loadConfigFile() require.NoError(t, err) assert.Equal(t, "TEST_ACCOUNT", m["default"].Account) @@ -49,8 +50,8 @@ func TestProfileConfig(t *testing.T) { configPath := testFile(t, "config", []byte(c)) t.Run("with found profile", func(t *testing.T) { - cleanupEnvVars := setupEnvVars(t, "", "", "", "", configPath) - t.Cleanup(cleanupEnvVars) + t.Setenv(snowflakeenvs.ConfigPath, configPath) + config, err := ProfileConfig("securityadmin") require.NoError(t, err) assert.Equal(t, "TEST_ACCOUNT", config.Account) @@ -60,33 +61,21 @@ func TestProfileConfig(t *testing.T) { }) t.Run("with not found profile", func(t *testing.T) { - cleanupEnvVars := setupEnvVars(t, "", "", "", "", configPath) - t.Cleanup(cleanupEnvVars) + t.Setenv(snowflakeenvs.ConfigPath, configPath) + config, err := ProfileConfig("orgadmin") require.NoError(t, err) require.Nil(t, config) }) -} -func TestEnvConfig(t *testing.T) { - t.Run("with no environment variables", func(t *testing.T) { - cleanupEnvVars := setupEnvVars(t, "", "", "", "", "") - t.Cleanup(cleanupEnvVars) - config := EnvConfig() - assert.Equal(t, "", config.Account) - assert.Equal(t, "", config.User) - assert.Equal(t, "", config.Password) - assert.Equal(t, "", config.Role) - }) + t.Run("with not found config", func(t *testing.T) { + dir, err := os.UserHomeDir() + require.NoError(t, err) + t.Setenv(snowflakeenvs.ConfigPath, dir) - t.Run("with environment variables", func(t *testing.T) { - cleanupEnvVars := setupEnvVars(t, "TEST_ACCOUNT", "TEST_USER", "abcd1234", "ACCOUNTADMIN", "") - t.Cleanup(cleanupEnvVars) - config := EnvConfig() - assert.Equal(t, "TEST_ACCOUNT", config.Account) - assert.Equal(t, "TEST_USER", config.User) - assert.Equal(t, "abcd1234", config.Password) - assert.Equal(t, "ACCOUNTADMIN", config.Role) + config, err := ProfileConfig("orgadmin") + require.Error(t, err) + require.Nil(t, config) }) } @@ -97,26 +86,3 @@ func testFile(t *testing.T, filename string, dat []byte) string { require.NoError(t, err) return path } - -func setupEnvVars(t *testing.T, account, user, password, role, configPath string) func() { - t.Helper() - orginalAccount := os.Getenv("SNOWFLAKE_ACCOUNT") - orginalUser := os.Getenv("SNOWFLAKE_USER") - originalPassword := os.Getenv("SNOWFLAKE_PASSWORD") - originalRole := os.Getenv("SNOWFLAKE_ROLE") - originalPath := os.Getenv("SNOWFLAKE_CONFIG_PATH") - - os.Setenv("SNOWFLAKE_ACCOUNT", account) - os.Setenv("SNOWFLAKE_USER", user) - os.Setenv("SNOWFLAKE_PASSWORD", password) - os.Setenv("SNOWFLAKE_ROLE", role) - os.Setenv("SNOWFLAKE_CONFIG_PATH", configPath) - - return func() { - os.Setenv("SNOWFLAKE_ACCOUNT", orginalAccount) - os.Setenv("SNOWFLAKE_USER", orginalUser) - os.Setenv("SNOWFLAKE_PASSWORD", originalPassword) - os.Setenv("SNOWFLAKE_ROLE", originalRole) - os.Setenv("SNOWFLAKE_CONFIG_PATH", originalPath) - } -} From f68b99450cf5a156424b91015454200ada41f016 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 14:12:50 +0100 Subject: [PATCH 07/14] Replace string usages of secondary profile --- pkg/resources/database_acceptance_test.go | 3 ++- ...privileges_to_account_role_acceptance_test.go | 16 ++++++++-------- pkg/sdk/helper_test.go | 10 ++++------ pkg/sdk/testint/setup_test.go | 7 ++----- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/pkg/resources/database_acceptance_test.go b/pkg/resources/database_acceptance_test.go index 45d6be2903..1f9a3c46e2 100644 --- a/pkg/resources/database_acceptance_test.go +++ b/pkg/resources/database_acceptance_test.go @@ -8,6 +8,7 @@ import ( acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-testing/config" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" @@ -228,7 +229,7 @@ func dropDatabaseOutsideTerraform(t *testing.T, id string) { func getSecondaryAccount(t *testing.T) string { t.Helper() - secondaryConfig, err := sdk.ProfileConfig("secondary_test_account") + secondaryConfig, err := sdk.ProfileConfig(testprofiles.Secondary) require.NoError(t, err) secondaryClient, err := sdk.NewClient(secondaryConfig) diff --git a/pkg/resources/grant_privileges_to_account_role_acceptance_test.go b/pkg/resources/grant_privileges_to_account_role_acceptance_test.go index a2c99d1327..1f8e6d260b 100644 --- a/pkg/resources/grant_privileges_to_account_role_acceptance_test.go +++ b/pkg/resources/grant_privileges_to_account_role_acceptance_test.go @@ -10,18 +10,18 @@ import ( "strings" "testing" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/hashicorp/terraform-plugin-testing/helper/acctest" - acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-testing/config" + "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/plancheck" "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-plugin-testing/tfversion" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAcc_GrantPrivilegesToAccountRole_OnAccount(t *testing.T) { @@ -931,7 +931,7 @@ func TestAcc_GrantPrivilegesToAccountRole_ImportedPrivileges(t *testing.T) { func getSecondaryAccountName(t *testing.T) (string, error) { t.Helper() - config, err := sdk.ProfileConfig("secondary_test_account") + config, err := sdk.ProfileConfig(testprofiles.Secondary) if err != nil { t.Fatal(err) } @@ -953,7 +953,7 @@ func getAccountName(t *testing.T) (string, error) { func createSharedDatabaseOnSecondaryAccount(t *testing.T, databaseName string, shareName string) error { t.Helper() - config, err := sdk.ProfileConfig("secondary_test_account") + config, err := sdk.ProfileConfig(testprofiles.Secondary) if err != nil { t.Fatal(err) } @@ -976,7 +976,7 @@ func createSharedDatabaseOnSecondaryAccount(t *testing.T, databaseName string, s func dropSharedDatabaseOnSecondaryAccount(t *testing.T, databaseName string, shareName string) error { t.Helper() - config, err := sdk.ProfileConfig("secondary_test_account") + config, err := sdk.ProfileConfig(testprofiles.Secondary) if err != nil { t.Fatal(err) } diff --git a/pkg/sdk/helper_test.go b/pkg/sdk/helper_test.go index c6b7c68f32..2de6670556 100644 --- a/pkg/sdk/helper_test.go +++ b/pkg/sdk/helper_test.go @@ -2,6 +2,8 @@ package sdk import ( "testing" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" ) func testClient(t *testing.T) *Client { @@ -15,16 +17,12 @@ func testClient(t *testing.T) *Client { return client } -const ( - secondaryAccountProfile = "secondary_test_account" -) - func testSecondaryClient(t *testing.T) *Client { t.Helper() - client, err := testClientFromProfile(t, secondaryAccountProfile) + client, err := testClientFromProfile(t, testprofiles.Secondary) if err != nil { - t.Skipf("Snowflake secondary account not configured. Must be set in ~./snowflake/config.yml with profile name: %s", secondaryAccountProfile) + t.Skipf("Snowflake secondary account not configured. Must be set in ~./snowflake/config.yml with profile name: %s", testprofiles.Secondary) } return client diff --git a/pkg/sdk/testint/setup_test.go b/pkg/sdk/testint/setup_test.go index 2a0a8b5ee6..945073ff69 100644 --- a/pkg/sdk/testint/setup_test.go +++ b/pkg/sdk/testint/setup_test.go @@ -7,14 +7,11 @@ import ( "testing" "time" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk/internal/random" ) -const ( - secondaryAccountProfile = "secondary_test_account" -) - var ( TestWarehouseName = "int_test_wh_" + random.UUID() TestDatabaseName = "int_test_db_" + random.UUID() @@ -116,7 +113,7 @@ func (itc *integrationTestContext) initialize() error { itc.warehouse = wh itc.warehouseCleanup = whCleanup - config, err := sdk.ProfileConfig(secondaryAccountProfile) + config, err := sdk.ProfileConfig(testprofiles.Secondary) if err != nil { return err } From d2727c5e1f3eb03d98cc0c1709255d021bb1cee9 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 14:30:50 +0100 Subject: [PATCH 08/14] Add merge config tests --- pkg/sdk/config_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/pkg/sdk/config_test.go b/pkg/sdk/config_test.go index 8e26b6a9dd..7ffcf15e59 100644 --- a/pkg/sdk/config_test.go +++ b/pkg/sdk/config_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" + "github.com/snowflakedb/gosnowflake" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -79,6 +80,57 @@ func TestProfileConfig(t *testing.T) { }) } +func Test_MergeConfig(t *testing.T) { + createConfig := func(user string, password string, account string, region string) *gosnowflake.Config { + return &gosnowflake.Config{ + User: user, + Password: password, + Account: account, + Region: region, + } + } + + t.Run("merge configs", func(t *testing.T) { + config1 := createConfig("user", "password", "account", "") + config2 := createConfig("user2", "", "", "region2") + + config := MergeConfig(config1, config2) + + require.Equal(t, "user", config.User) + require.Equal(t, "password", config.Password) + require.Equal(t, "account", config.Account) + require.Equal(t, "region2", config.Region) + require.Equal(t, "", config.Role) + + require.Equal(t, config1, config) + require.Equal(t, "user", config1.User) + require.Equal(t, "password", config1.Password) + require.Equal(t, "account", config1.Account) + require.Equal(t, "region2", config1.Region) + require.Equal(t, "", config1.Role) + }) + + t.Run("merge configs reverted", func(t *testing.T) { + config1 := createConfig("user", "password", "account", "") + config2 := createConfig("user2", "", "", "region2") + + config := MergeConfig(config2, config1) + + require.Equal(t, "user2", config.User) + require.Equal(t, "password", config.Password) + require.Equal(t, "account", config.Account) + require.Equal(t, "region2", config.Region) + require.Equal(t, "", config.Role) + + require.Equal(t, config2, config) + require.Equal(t, "user2", config2.User) + require.Equal(t, "password", config2.Password) + require.Equal(t, "account", config2.Account) + require.Equal(t, "region2", config2.Region) + require.Equal(t, "", config2.Role) + }) +} + func testFile(t *testing.T, filename string, dat []byte) string { t.Helper() path := filepath.Join(t.TempDir(), filename) From 70786888fb4419a17fa8fdb64469a6453351bbf1 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 14:56:01 +0100 Subject: [PATCH 09/14] Test get or skip --- pkg/acceptance/testenvs/testenvs_test.go | 43 +++++++++++++++++++ .../testenvs/testing_environment_variables.go | 1 - 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 pkg/acceptance/testenvs/testenvs_test.go diff --git a/pkg/acceptance/testenvs/testenvs_test.go b/pkg/acceptance/testenvs/testenvs_test.go new file mode 100644 index 0000000000..a418aef438 --- /dev/null +++ b/pkg/acceptance/testenvs/testenvs_test.go @@ -0,0 +1,43 @@ +package testenvs_test + +import ( + "sync" + "testing" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" + "github.com/stretchr/testify/require" +) + +func Test_GetOrSkipTest(t *testing.T) { + runGetOrSkipInGoroutineAndWait := func(tut *testing.T) string { + var env string + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + env = testenvs.GetOrSkipTest(tut, testenvs.User) + }() + wg.Wait() + return env + } + + t.Run("skip test if missing", func(t *testing.T) { + t.Setenv(string(testenvs.User), "") + + tut := &testing.T{} + env := runGetOrSkipInGoroutineAndWait(tut) + + require.True(t, tut.Skipped()) + require.Empty(t, env) + }) + + t.Run("get env if exists", func(t *testing.T) { + t.Setenv(string(testenvs.User), "user") + + tut := &testing.T{} + env := runGetOrSkipInGoroutineAndWait(tut) + + require.False(t, tut.Skipped()) + require.Equal(t, "user", env) + }) +} diff --git a/pkg/acceptance/testenvs/testing_environment_variables.go b/pkg/acceptance/testenvs/testing_environment_variables.go index 2e3f3d6bee..068141b410 100644 --- a/pkg/acceptance/testenvs/testing_environment_variables.go +++ b/pkg/acceptance/testenvs/testing_environment_variables.go @@ -16,7 +16,6 @@ const ( Host env = "TEST_SF_TF_HOST" ) -// TODO: test func GetOrSkipTest(t *testing.T, envName Env) string { env := os.Getenv(fmt.Sprintf("%v", envName)) if env == "" { From a1e04096b5f705959af3e5975f7b1de9e34e8315 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 15:04:51 +0100 Subject: [PATCH 10/14] Test assertions --- .../testenvs/{helpers.go => assertions.go} | 4 +- pkg/acceptance/testenvs/testenvs_test.go | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) rename pkg/acceptance/testenvs/{helpers.go => assertions.go} (50%) diff --git a/pkg/acceptance/testenvs/helpers.go b/pkg/acceptance/testenvs/assertions.go similarity index 50% rename from pkg/acceptance/testenvs/helpers.go rename to pkg/acceptance/testenvs/assertions.go index 9ee7333b07..f8ef7c7997 100644 --- a/pkg/acceptance/testenvs/helpers.go +++ b/pkg/acceptance/testenvs/assertions.go @@ -8,9 +8,9 @@ import ( ) func AssertEnvNotSet(t *testing.T, envName string) { - require.Empty(t, os.Getenv(envName)) + require.Emptyf(t, os.Getenv(envName), "environment variable %v should not be set", envName) } func AssertEnvSet(t *testing.T, envName string) { - require.NotEmpty(t, os.Getenv(envName)) + require.NotEmptyf(t, os.Getenv(envName), "environment variable %v should not be empty", envName) } diff --git a/pkg/acceptance/testenvs/testenvs_test.go b/pkg/acceptance/testenvs/testenvs_test.go index a418aef438..de8db8879c 100644 --- a/pkg/acceptance/testenvs/testenvs_test.go +++ b/pkg/acceptance/testenvs/testenvs_test.go @@ -41,3 +41,41 @@ func Test_GetOrSkipTest(t *testing.T) { require.Equal(t, "user", env) }) } + +func Test_Assertions(t *testing.T) { + runAssertionInGoroutineAndWait := func(assertion func()) { + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + assertion() + }() + wg.Wait() + } + + t.Run("test if env does not exist", func(t *testing.T) { + t.Setenv(string(testenvs.User), "") + + tut1 := &testing.T{} + runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) }) + + tut2 := &testing.T{} + runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) }) + + require.False(t, tut1.Failed()) + require.True(t, tut2.Failed()) + }) + + t.Run("test if env exists", func(t *testing.T) { + t.Setenv(string(testenvs.User), "user") + + tut1 := &testing.T{} + runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) }) + + tut2 := &testing.T{} + runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) }) + + require.True(t, tut1.Failed()) + require.False(t, tut2.Failed()) + }) +} From 2c3bb137d9570943cd81c7b508914101f12ee7d9 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 15:08:53 +0100 Subject: [PATCH 11/14] Run linters --- pkg/acceptance/testenvs/assertions.go | 2 ++ pkg/acceptance/testenvs/testenvs_test.go | 5 +++-- .../testenvs/testing_environment_variables.go | 3 ++- .../testprofiles/testing_config_profiles.go | 8 +++++--- .../snowflake_environment_variables.go | 14 ++++++++------ pkg/provider/provider_acceptance_test.go | 2 +- pkg/sdk/client_integration_test.go | 2 +- 7 files changed, 22 insertions(+), 14 deletions(-) diff --git a/pkg/acceptance/testenvs/assertions.go b/pkg/acceptance/testenvs/assertions.go index f8ef7c7997..77f0caf4d0 100644 --- a/pkg/acceptance/testenvs/assertions.go +++ b/pkg/acceptance/testenvs/assertions.go @@ -8,9 +8,11 @@ import ( ) func AssertEnvNotSet(t *testing.T, envName string) { + t.Helper() require.Emptyf(t, os.Getenv(envName), "environment variable %v should not be set", envName) } func AssertEnvSet(t *testing.T, envName string) { + t.Helper() require.NotEmptyf(t, os.Getenv(envName), "environment variable %v should not be empty", envName) } diff --git a/pkg/acceptance/testenvs/testenvs_test.go b/pkg/acceptance/testenvs/testenvs_test.go index de8db8879c..65b64eab1b 100644 --- a/pkg/acceptance/testenvs/testenvs_test.go +++ b/pkg/acceptance/testenvs/testenvs_test.go @@ -9,13 +9,14 @@ import ( ) func Test_GetOrSkipTest(t *testing.T) { - runGetOrSkipInGoroutineAndWait := func(tut *testing.T) string { + runGetOrSkipInGoroutineAndWait := func(t *testing.T) string { + t.Helper() var env string var wg sync.WaitGroup wg.Add(1) go func() { defer wg.Done() - env = testenvs.GetOrSkipTest(tut, testenvs.User) + env = testenvs.GetOrSkipTest(t, testenvs.User) }() wg.Wait() return env diff --git a/pkg/acceptance/testenvs/testing_environment_variables.go b/pkg/acceptance/testenvs/testing_environment_variables.go index 068141b410..11882e8715 100644 --- a/pkg/acceptance/testenvs/testing_environment_variables.go +++ b/pkg/acceptance/testenvs/testing_environment_variables.go @@ -10,13 +10,14 @@ type env string const ( User env = "TEST_SF_TF_USER" - Password env = "TEST_SF_TF_PASSWORD" + Password env = "TEST_SF_TF_PASSWORD" // #nosec G101 Account env = "TEST_SF_TF_ACCOUNT" Role env = "TEST_SF_TF_ROLE" Host env = "TEST_SF_TF_HOST" ) func GetOrSkipTest(t *testing.T, envName Env) string { + t.Helper() env := os.Getenv(fmt.Sprintf("%v", envName)) if env == "" { t.Skipf("Skipping %s, env %v missing", t.Name(), envName) diff --git a/pkg/acceptance/testprofiles/testing_config_profiles.go b/pkg/acceptance/testprofiles/testing_config_profiles.go index b9b9b90e8a..0fbef504ca 100644 --- a/pkg/acceptance/testprofiles/testing_config_profiles.go +++ b/pkg/acceptance/testprofiles/testing_config_profiles.go @@ -1,5 +1,7 @@ package testprofiles -const Default = "default" -const Secondary = "secondary_test_account" -const IncorrectUserAndPassword = "incorrect_test_profile" +const ( + Default = "default" + Secondary = "secondary_test_account" + IncorrectUserAndPassword = "incorrect_test_profile" +) diff --git a/pkg/internal/snowflakeenvs/snowflake_environment_variables.go b/pkg/internal/snowflakeenvs/snowflake_environment_variables.go index 916d4f4898..8c6d21101c 100644 --- a/pkg/internal/snowflakeenvs/snowflake_environment_variables.go +++ b/pkg/internal/snowflakeenvs/snowflake_environment_variables.go @@ -1,8 +1,10 @@ package snowflakeenvs -const Account = "SNOWFLAKE_ACCOUNT" -const User = "SNOWFLAKE_USER" -const Password = "SNOWFLAKE_PASSWORD" -const Role = "SNOWFLAKE_ROLE" -const ConfigPath = "SNOWFLAKE_CONFIG_PATH" -const Host = "SNOWFLAKE_HOST" +const ( + Account = "SNOWFLAKE_ACCOUNT" + User = "SNOWFLAKE_USER" + Password = "SNOWFLAKE_PASSWORD" + Role = "SNOWFLAKE_ROLE" + ConfigPath = "SNOWFLAKE_CONFIG_PATH" + Host = "SNOWFLAKE_HOST" +) diff --git a/pkg/provider/provider_acceptance_test.go b/pkg/provider/provider_acceptance_test.go index 214ebda1e7..ef4ce1bf93 100644 --- a/pkg/provider/provider_acceptance_test.go +++ b/pkg/provider/provider_acceptance_test.go @@ -2,7 +2,6 @@ package provider_test import ( "fmt" - "github.com/stretchr/testify/require" "os" "regexp" "testing" @@ -14,6 +13,7 @@ import ( "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/tfversion" + "github.com/stretchr/testify/require" ) func TestAcc_Provider_configHierarchy(t *testing.T) { diff --git a/pkg/sdk/client_integration_test.go b/pkg/sdk/client_integration_test.go index 84d5c17005..9498d6b2d7 100644 --- a/pkg/sdk/client_integration_test.go +++ b/pkg/sdk/client_integration_test.go @@ -3,10 +3,10 @@ package sdk import ( "context" "database/sql" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "os" "testing" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/snowflakeenvs" "github.com/snowflakedb/gosnowflake" From c4eec4634c01a46f776d2c54c87d5c66fc6f2478 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 15:22:17 +0100 Subject: [PATCH 12/14] Add note to the migration guide --- MIGRATION_GUIDE.md | 16 ++++++++++++++++ docs/index.md | 2 +- templates/index.md.tmpl | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index 3df6788252..7eb2a326fb 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -5,6 +5,22 @@ describe deprecations or breaking changes and help you to change your configurat across different versions. ## v0.86.0 ➞ v0.87.0 +### Provider configuration changes + +#### **IMPORTANT** *(bug fix)* Configuration hierarchy +There were several issues reported about the configuration hierarchy, e.g. [#2294](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2294) and [#2242](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2242). +In fact, the order of precedence described in the docs was not followed. This have led to the incorrect behavior. + +After migrating to this version, the hierarchy from the docs should be followed: +```text +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 +``` + +**BEWARE**: your configurations will be affected with that change because they may have been leveraging the incorrect configurations precedence. Please be sure to check all the configurations before running terraform. + ### snowflake_failover_group resource changes #### *(bug fix)* ACCOUNT PARAMETERS is returned as PARAMETERS from SHOW FAILOVER GROUPS Longer context in [#2517](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2517). diff --git a/docs/index.md b/docs/index.md index 77fe01af68..a25c829ca1 100644 --- a/docs/index.md +++ b/docs/index.md @@ -7,7 +7,7 @@ description: Manage SnowflakeDB with Terraform. ~> **Disclaimer** the project is still in the 0.x.x version, which means it’s still in the experimental phase (check [Go module versioning](https://go.dev/doc/modules/version-numbers#v0-number) for more details). It can be used in production but makes no stability or backward compatibility guarantees. We do not provide backward bug fixes and, therefore, always suggest using the newest version. We are providing only limited support for the provider; priorities will be assigned on a case-by-case basis. Our main current goals are stabilization, addressing existing issues, and providing the missing features (prioritizing the GA features; supporting PrPr and PuPr features are not high priorities now). With all that in mind, we aim to reach V1 with a stable, reliable, and functional provider. V1 will be free of all the above limitations. --> **Note** Please check the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md) when changing the version of the provider. +~> **Note** Please check the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md) when changing the version of the provider. -> **Note** the current roadmap is available in our GitHub repository: [ROADMAP.md](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md). diff --git a/templates/index.md.tmpl b/templates/index.md.tmpl index 28bae7f604..bbd6ca78e1 100644 --- a/templates/index.md.tmpl +++ b/templates/index.md.tmpl @@ -7,7 +7,7 @@ description: Manage SnowflakeDB with Terraform. ~> **Disclaimer** the project is still in the 0.x.x version, which means it’s still in the experimental phase (check [Go module versioning](https://go.dev/doc/modules/version-numbers#v0-number) for more details). It can be used in production but makes no stability or backward compatibility guarantees. We do not provide backward bug fixes and, therefore, always suggest using the newest version. We are providing only limited support for the provider; priorities will be assigned on a case-by-case basis. Our main current goals are stabilization, addressing existing issues, and providing the missing features (prioritizing the GA features; supporting PrPr and PuPr features are not high priorities now). With all that in mind, we aim to reach V1 with a stable, reliable, and functional provider. V1 will be free of all the above limitations. --> **Note** Please check the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md) when changing the version of the provider. +~> **Note** Please check the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md) when changing the version of the provider. -> **Note** the current roadmap is available in our GitHub repository: [ROADMAP.md](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md). From 50bbfa78b55d13bc7db795eb88490763638fbd27 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 23 Feb 2024 15:42:53 +0100 Subject: [PATCH 13/14] Trigger build From a2e4994ee909f71592e9b8a29ab85a97ec99f700 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 26 Feb 2024 12:34:32 +0100 Subject: [PATCH 14/14] Fix after reviews --- pkg/acceptance/testenvs/testenvs_test.go | 20 ++++++++++++-------- pkg/sdk/config_test.go | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/pkg/acceptance/testenvs/testenvs_test.go b/pkg/acceptance/testenvs/testenvs_test.go index 65b64eab1b..f5ddd8c645 100644 --- a/pkg/acceptance/testenvs/testenvs_test.go +++ b/pkg/acceptance/testenvs/testenvs_test.go @@ -9,7 +9,9 @@ import ( ) func Test_GetOrSkipTest(t *testing.T) { - runGetOrSkipInGoroutineAndWait := func(t *testing.T) string { + // runGetOrSkipInGoroutineAndWaitForCompletion is needed because underneath we test t.Skipf, that leads to t.SkipNow() that in turn call runtime.Goexit() + // so we need to be wrapped in a Goroutine. + runGetOrSkipInGoroutineAndWaitForCompletion := func(t *testing.T) string { t.Helper() var env string var wg sync.WaitGroup @@ -26,7 +28,7 @@ func Test_GetOrSkipTest(t *testing.T) { t.Setenv(string(testenvs.User), "") tut := &testing.T{} - env := runGetOrSkipInGoroutineAndWait(tut) + env := runGetOrSkipInGoroutineAndWaitForCompletion(tut) require.True(t, tut.Skipped()) require.Empty(t, env) @@ -36,7 +38,7 @@ func Test_GetOrSkipTest(t *testing.T) { t.Setenv(string(testenvs.User), "user") tut := &testing.T{} - env := runGetOrSkipInGoroutineAndWait(tut) + env := runGetOrSkipInGoroutineAndWaitForCompletion(tut) require.False(t, tut.Skipped()) require.Equal(t, "user", env) @@ -44,7 +46,9 @@ func Test_GetOrSkipTest(t *testing.T) { } func Test_Assertions(t *testing.T) { - runAssertionInGoroutineAndWait := func(assertion func()) { + // runAssertionInGoroutineAndWaitForCompletion is needed because underneath we test require, that leads to t.FailNow() that in turn call runtime.Goexit() + // so we need to be wrapped in a Goroutine. + runAssertionInGoroutineAndWaitForCompletion := func(assertion func()) { var wg sync.WaitGroup wg.Add(1) go func() { @@ -58,10 +62,10 @@ func Test_Assertions(t *testing.T) { t.Setenv(string(testenvs.User), "") tut1 := &testing.T{} - runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) }) + runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) }) tut2 := &testing.T{} - runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) }) + runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) }) require.False(t, tut1.Failed()) require.True(t, tut2.Failed()) @@ -71,10 +75,10 @@ func Test_Assertions(t *testing.T) { t.Setenv(string(testenvs.User), "user") tut1 := &testing.T{} - runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) }) + runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvNotSet(tut1, string(testenvs.User)) }) tut2 := &testing.T{} - runAssertionInGoroutineAndWait(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) }) + runAssertionInGoroutineAndWaitForCompletion(func() { testenvs.AssertEnvSet(tut2, string(testenvs.User)) }) require.True(t, tut1.Failed()) require.False(t, tut2.Failed()) diff --git a/pkg/sdk/config_test.go b/pkg/sdk/config_test.go index 7ffcf15e59..d34d812a88 100644 --- a/pkg/sdk/config_test.go +++ b/pkg/sdk/config_test.go @@ -110,7 +110,7 @@ func Test_MergeConfig(t *testing.T) { require.Equal(t, "", config1.Role) }) - t.Run("merge configs reverted", func(t *testing.T) { + t.Run("merge configs inverted", func(t *testing.T) { config1 := createConfig("user", "password", "account", "") config2 := createConfig("user2", "", "", "region2")