feat: Parameters sdk v2 #1914

+}
+
+func (parameters *parameters) SetAccountParameter(ctx context.Context, parameter AccountParameter, value string) error {
+	opts := AlterAccountOptions{Set: &AccountSet{Parameters: &AccountLevelParameters{AccountParameters: &AccountParameters{}}}}


This is hard to read, can you split this over multiple lines?

pkg/resources/session_parameter.go

sfc-gh-ngaberel · 2023-06-29T21:18:54Z

pkg/resources/session_parameter.go

+		err := client.Parameters.SetSessionParameterForAccount(ctx, parameter, value)
+		if err != nil {
+			return err
+		}
 	} else {
 		if user == "" {
 			return fmt.Errorf("user is required if on_account is false")


Can we move this logic to a validation function in the resource schema?

I'm not sure if we can do that since we only check if it's empty if on_account is false. validation functions in resource schema cannot access another attribute value afaik

in the tf plugin framework it is possible to use a plan modifier to validate this as Nathan is describing. it is something we should revisit, for sure

sfc-gh-ngaberel · 2023-06-29T21:19:38Z

pkg/resources/session_parameter.go

+		if reflect.TypeOf(parameterDefault.DefaultValue) == typeString {
+			value = fmt.Sprintf("'%s'", snowflake.EscapeString(value))
+		}
+		builder := snowflake.NewSessionParameter(key, value, db)


Still using the old SDK here.

sfc-gh-ngaberel · 2023-06-29T21:20:59Z

pkg/resources/session_parameter.go

+		typeString := reflect.TypeOf("")
+		if reflect.TypeOf(parameterDefault.DefaultValue) == typeString {
+			value = fmt.Sprintf("'%s'", snowflake.EscapeString(value))
+		}


This should already be handled by the new SDK, cf. https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/pkg/sdk/sql_builder.go#L52

pkg/resources/session_parameter.go

sfc-gh-ngaberel · 2023-06-29T21:25:37Z

pkg/resources/session_parameter.go

+		if user == "" {
+			return fmt.Errorf("user is required if on_account is false")
+		}
+		parameterDefault := snowflake.GetParameterDefaults(snowflake.ParameterTypeSession)[key]


Remove dependency on old SDK.

sfc-gh-ngaberel · 2023-06-29T21:26:45Z

pkg/resources/object_parameter.go

@@ -91,6 +93,9 @@ func CreateObjectParameter(d *schema.ResourceData, meta interface{}) error {
 	db := meta.(*sql.DB)
 	key := d.Get("key").(string)
 	value := d.Get("value").(string)
+	client := sdk.NewClientFromDB(db)
+	ctx := context.Background()
+	parameter := sdk.ObjectParameter(key)

 	parameterDefault := snowflake.GetParameterDefaults(snowflake.ParameterTypeObject)[key]


Old SDK here too.

I don't think we can get rid of that in this case, at least not fully. In order to make use of new sdk here, we need to have new sdk implementation for all objects which can take parameters and that's not the case just yet. It can be improved in the future, but I think it has to use old sdk for now

sfc-gh-ngaberel · 2023-06-29T21:27:17Z

pkg/resources/account_parameter.go


-	parameterDefault := snowflake.GetParameterDefaults(snowflake.ParameterTypeAccount)[key]
+	parameterDefault := snowflake.GetParameterDefaults(snowflake.ParameterTypeSession)[key]


Old SDK here.

sfc-gh-ngaberel · 2023-06-29T21:27:54Z

pkg/resources/account_parameter_acceptance_test.go

-func TestAcc_AccountParameter_CLIENT_METADATA_USE_SESSION_DATABASE(t *testing.T) {
-	resource.ParallelTest(t, resource.TestCase{
-		Providers:    providers(),
-		CheckDestroy: nil,
-		Steps: []resource.TestStep{
-			{
-				Config: accountParameterBasic("CLIENT_METADATA_USE_SESSION_DATABASE", "true"),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr("snowflake_account_parameter.p", "key", "CLIENT_METADATA_USE_SESSION_DATABASE"),
-					resource.TestCheckResourceAttr("snowflake_account_parameter.p", "value", "true"),
-				),
-			},
-		},
-	})
-}


Why are we removing this test? It should still pass, right?

This test tries to set CLIENT_METADATA_USE_SESSION_DATABASE for account.
CLIENT_METADATA_USE_SESSION_DATABASE is a session parameter and I think we should only allow to set it using a session_parameter. For now I modified it to use an actual account parameter, but if you think we should allow to set session parameters using account_parameter resource then we can discuss it

sfc-gh-swinkler · 2023-07-06T03:52:19Z

pkg/resources/account_parameter.go

 	if err != nil {
-		return fmt.Errorf("error reading account parameter err = %w", err)
+		return fmt.Errorf("error creating account parameter err = %w", err)


this should probably say "error resetting account parameter to default value"

pkg/resources/session_parameter.go

github-actions · 2023-07-24T21:42:01Z

Integration tests success for 555e6f156f276102504ea164f90c1260c9082775

github-actions · 2023-07-27T18:05:25Z

Integration tests failure for d2b679e1cc53bf4a943bf83bc062d04df4ca16dc

…rm-provider-snowflake into parameters-sdk-v2

…nowflake into parameters-sdk-v2

github-actions · 2023-08-26T16:16:33Z

Integration tests failure for 98f91238b1bc918acee47e7cfbd7fd11dd3be7d4

pkg/sdk/parameters.go

pkg/sdk/validation_test.go

pkg/datasources/parameters.go

pkg/resources/database_grant.go

pkg/resources/external_table_grant.go

github-actions · 2023-08-26T17:39:21Z

Integration tests failure for ad693953fadd6c92212f6260607504b7d855b4c9

github-actions · 2023-08-26T18:00:42Z

Integration tests failure for 44283b2a181d7b69208ec969bf91df40223a0cb0

github-actions · 2023-09-11T03:06:20Z

Integration tests failure for b45d015595bf3d3755037e77676d63cd3a6e55a7

github-actions · 2023-09-11T16:45:47Z

Integration tests failure for a306eadc47dc57d4c0f1810d94b0ae56613decbb

github-actions · 2023-09-11T16:48:42Z

Integration tests failure for 8411935d901d257ffe3fdf73bb7246f8d3fcabd3

github-actions · 2023-09-11T17:43:24Z

Integration tests failure for b41d4d4a80243af477d7d5675ddba56bd83e20d2

github-actions · 2023-09-11T18:03:57Z

Integration tests failure for 6e4883eaf195e9fc4630f43decde18beb89cb89b

github-actions · 2023-09-11T18:19:01Z

Integration tests failure for 76a384754b02267502dbda6d786a48b53299f1a6

github-actions · 2023-09-11T19:34:57Z

Integration tests failure for 3665a78bb563c3746cae573e8e3fb30258905c94

github-actions · 2023-09-11T23:09:54Z

Integration tests success for 3ae991be2c7d81813dabf68bbaec7a29fb5af733

github-actions · 2023-09-11T23:30:58Z

Integration tests success for 4c091ac2585c8178a9cde37f9c535fe9fba62115

sfc-gh-swinkler · 2023-09-07T21:06:39Z

pkg/resources/session_parameter.go

+		err := client.Parameters.SetSessionParameterForAccount(ctx, parameter, value)
+		if err != nil {
+			return err
+		}
 	} else {
 		if user == "" {
 			return fmt.Errorf("user is required if on_account is false")


in the tf plugin framework it is possible to use a plan modifier to validate this as Nathan is describing. it is something we should revisit, for sure

sfc-gh-swinkler · 2023-09-07T21:13:34Z

pkg/sdk/parameters.go

+	SetSessionParameterForAccount(ctx context.Context, parameter SessionParameter, value string) error
+	SetSessionParameterForUser(ctx context.Context, id AccountObjectIdentifier, parameter SessionParameter, value string) error
+	SetObjectParameterForAccount(ctx context.Context, parameter ObjectParameter, value string) error
+	SetObjectParameterForUser(ctx context.Context, id AccountObjectIdentifier, parameter ObjectParameter, value string) error


this makes no sense. you cannot set an object parameter for a user. Object parameters can only be set for objects or at the account level. Perhaps this should be renamed to SetObjectParameterForObject

sfc-gh-swinkler · 2023-09-07T21:54:46Z

pkg/sdk/parameters.go

+	return nil
+}
+
+func (parameters *parameters) SetSessionParameterForUser(ctx context.Context, id AccountObjectIdentifier, parameter SessionParameter, value string) error {


this code is copypasted from above. maybe should be refactored?

sfc-gh-swinkler · 2023-09-07T21:55:15Z

pkg/sdk/parameters.go

+func (parameters *parameters) SetObjectParameterForUser(ctx context.Context, id AccountObjectIdentifier, parameter ObjectParameter, value string) error {
+	opts := AlterUserOptions{Set: &UserSet{ObjectParameters: &UserObjectParameters{}}}
+	switch parameter {
+	case ObjectParameterNetworkPolicy:


both of these are actually session parameters

sfc-gh-swinkler · 2023-09-07T22:00:55Z

pkg/sdk/parameters.go

+	AllowedObjectTypes []ObjectType
+}
+
+// ParameterDefaults returns a map of default values for all parameters.


these parameter defaults feel clunky

sfc-gh-swinkler · 2023-09-07T22:02:55Z

pkg/sdk/parameters.go

+	return ParameterDefaults()[key]
+}
+
+func parseBooleanParameter(parameter, value string) (_ *bool, err error) {


this could be any kind of parse function, no?

sfc-gh-pbosak added 3 commits June 22, 2023 08:30

update session_parameter

8c5ad79

adds missing session parameters

1a304b8

Merge branch 'main' of github.com:Snowflake-Labs/terraform-provider-s…

2b65611

…nowflake into parameters-sdk-v2

github-actions bot reviewed Jun 27, 2023

View reviewed changes

pkg/resources/account_parameter.go Outdated Show resolved Hide resolved

pkg/resources/account_parameter.go Outdated Show resolved Hide resolved

pkg/resources/account_parameter.go Outdated Show resolved Hide resolved

pkg/resources/session_parameter.go Outdated Show resolved Hide resolved

updatees object parameter to use new sdk implementation

3dd4b1e

sfc-gh-pbosak changed the title ~~Parameters sdk v2~~ feat: Parameters sdk v2 Jun 28, 2023

fixes tests

5aa65eb

add support for prevent_inline_from_url parameter

fbf0e75

adds support for enable_unredacted_query_syntax_error

1807dda

fixes formatting

f8d2ffa

fixes returning null instead of error

0e88f4f

sfc-gh-pbosak marked this pull request as ready for review June 28, 2023 13:44

sfc-gh-ngaberel suggested changes Jun 29, 2023

View reviewed changes

sfc-gh-swinkler requested changes Jul 6, 2023

View reviewed changes

Merge branch 'main' into parameters-sdk-v2

555e6f1

Merge branch 'main' into parameters-sdk-v2

d2b679e

sfc-gh-pbosak added 3 commits August 26, 2023 18:13

stuff

9a74772

Merge branch 'parameters-sdk-v2' of github.com:Snowflake-Labs/terrafo…

d6131bc

…rm-provider-snowflake into parameters-sdk-v2

Merge branch 'main' of github.com:Snowflake-Labs/terraform-provider-s…

98f9123

…nowflake into parameters-sdk-v2

github-actions bot reviewed Aug 26, 2023

View reviewed changes

changes

ad69395

github-actions bot reviewed Aug 26, 2023

View reviewed changes

pkg/sdk/validation_test.go Outdated Show resolved Hide resolved

pkg/datasources/parameters.go Outdated Show resolved Hide resolved

pkg/resources/database_grant.go Outdated Show resolved Hide resolved

pkg/resources/external_table_grant.go Outdated Show resolved Hide resolved

fixes

44283b2

update parameters

b45d015

sfc-gh-swinkler added 2 commits September 11, 2023 09:34

Merge branch 'main' into parameters-sdk-v2

8411935

update parameters

a306ead

sfc-gh-swinkler added 3 commits September 11, 2023 10:32

update parameters

831609c

update parameters

a85a3fb

update parameters

b41d4d4

update parameters

6e4883e

update parameters

76a3847

fix int test

3665a78

update parameters

3ae991b

update parameters

4c091ac

sfc-gh-swinkler approved these changes Sep 12, 2023

View reviewed changes

sfc-gh-swinkler merged commit 1d15355 into main Sep 12, 2023
7 of 8 checks passed

sfc-gh-swinkler deleted the parameters-sdk-v2 branch September 12, 2023 22:56

snowflake-release-please bot mentioned this pull request Sep 8, 2023

chore(main): release 0.71.0 #2049

Merged


		parameterDefault := snowflake.GetParameterDefaults(snowflake.ParameterTypeAccount)[key]
		parameterDefault := snowflake.GetParameterDefaults(snowflake.ParameterTypeSession)[key]

feat: Parameters sdk v2 #1914

feat: Parameters sdk v2 #1914

Conversation

sfc-gh-pbosak commented Jun 27, 2023 • edited

github-actions bot commented Jun 27, 2023

github-actions bot commented Jun 27, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

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

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

github-actions bot commented Jul 24, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Aug 26, 2023

github-actions bot commented Aug 26, 2023

github-actions bot commented Aug 26, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

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-pbosak commented Jun 27, 2023 •

edited