feat: Add secret to sdk #3091

sfc-gh-fbudzynski · 2024-09-19T09:02:16Z

Changes

Changed name of Secret struct in common_types.go due to naming clash
Added Secret Object to SDK
Unit and integration tests

References

CREATE SECRET

github-actions · 2024-09-19T10:42:02Z

Integration tests failure for 01a70062a3b2aed8351229d43d94e520ec3c1e05

…aming clash

github-actions · 2024-09-19T11:16:54Z

Integration tests failure for 929c810a42e5eade5724b107bb10432ed35e376b

github-actions · 2024-09-19T13:31:10Z

Integration tests failure for 8c5dcfb3ad8ad5d7394d32c8c8f6896697c5e498

sfc-gh-jcieslak · 2024-09-20T07:50:22Z

pkg/acceptance/helpers/secret_client.go

+	return c.context.client.Secrets
+}
+
+func (c *SecretClient) CreateSecreteWithBasicFlow(t *testing.T, id sdk.SchemaObjectIdentifier, username, password string) (*sdk.Secret, func()) {


Writing error: "Secrete". Usually in testing client you don't have to emphasize that it's for Secret when it's already on SecretClient, the function may be renamed to just CreateWithBasicFlow. Same for CleanupSecretFunc where it can be just CleanupFunc or better DropFunc.

pkg/sdk/secrets_def.go

sfc-gh-jcieslak · 2024-09-20T08:07:55Z

pkg/sdk/secrets_def.go

+		IfNotExists().
+		Name().
+		PredefinedQueryStructField("Type", "string", g.StaticOptions().SQL("TYPE = OAUTH2")).
+		Identifier("SecurityIntegration", g.KindOfT[AccountObjectIdentifier](), g.IdentifierOptions().Required().Equals().SQL("API_AUTHENTICATION").Required()).


I would rename this field to ApiIntegration (same for other types of Create). Security integration is a very general term and can mean many types of objects ApiIntegration would be a specific security integration that can be used with this secret. Plus, you can remove one of those .Require(), calling it once is enough.

sfc-gh-jcieslak · 2024-09-20T08:15:26Z

pkg/sdk/secrets_def.go

+		OptionalComment().
+		WithValidation(g.ValidIdentifier, "name").
+		WithValidation(g.ConflictingFields, "OrReplace", "IfNotExists"),
+	secretsSecurityIntegrationScopeDef,


You also have it in the operation above. Wouldn't it produce the same struct (SecurityIntegrationScope) twice in the generated output? As far as I remember it should work like that, but maybe in the meantime we added some kind of filtering to prevent it from happening. Oh ok, there's filtering, even I did it 😅

Should I remove the duplicate regardless of filtering or leave it as is?

sfc-gh-jcieslak · 2024-09-20T08:27:39Z

pkg/sdk/secrets_def.go

+		Name().
+		WithValidation(g.ValidIdentifier, "name"),
+).ShowOperation(
+	"https://docs.snowflake.com/en/sql-reference/sql/show-secret",


it should be show-secrets

sfc-gh-jcieslak · 2024-09-20T09:08:51Z

pkg/sdk/testint/secrets_gen_integration_test.go

+	})
+
+	// regarding the https://docs.snowflake.com/en/sql-reference/sql/create-secret secret should inherit security_integration scopes, but it does not do so
+	t.Run("Create: SecretWithOAuthClientCredentialsFlow - No Scopes Specified", func(t *testing.T) {


Good test 👍

sfc-gh-jcieslak · 2024-09-20T09:12:32Z

pkg/sdk/testint/secrets_gen_integration_test.go

+
+	integrationId := testClientHelper().Ids.RandomAccountObjectIdentifier()
+
+	refreshTokenExpiryTime := time.Now().Add(Day).Format(time.DateOnly)


Did you try other formats? How Snowflake behaves when we give it a different one? You can check manually.

sfc-gh-jcieslak · 2024-09-20T09:18:02Z

pkg/sdk/testint/secrets_gen_integration_test.go

+		require.NotContains(t, returnedSecrets, *secret2)
+	})
+
+	t.Run("Show: SecretWithGenericString with Like", func(t *testing.T) {


I wouldn't go that deep to test SHOW LIKE and SHOW IN for every secret type, but if we already have it lets leave it.

sfc-gh-jcieslak · 2024-09-20T09:24:58Z

pkg/sdk/testint/secrets_gen_integration_test.go

+	})
+
+	t.Run("ShowByID", func(t *testing.T) {
+		_, id := createSecretWithGenericString(t, "foo", nil)


To really test ShowById the best way would be to create a new schema on top of test database (you can do it with testClientHelper().Schema.CreateSchemaInDatabase(t, testClientHelper().Ids.DatabaseId())) and then create a secret with the same name as the first one in this newly created schema, and then see what ShowById returns. Take a look at this test "show by id - same name in different schemas", as a good example.

The test seems similar to what you have at the bottom of this file, but not quite.

sfc-gh-jcieslak · 2024-09-20T09:26:30Z

pkg/sdk/testint/secrets_gen_integration_test.go

+		return secret
+	}
+
+	t.Run("Show with In - same id in different schemas", func(t *testing.T) {


You have a lot of SHOW tests, isn't this case covered in one of them? You can also test it in the main function, no need to have it in TestInt_SecretsShowWithIn.

sfc-gh-jmichalak

Overall a good first PR 👍

sfc-gh-jmichalak · 2024-09-19T12:18:12Z

pkg/acceptance/helpers/secret_client.go

+	return secret, c.CleanupSecretFunc(t, id)
+}
+
+func (c *SecretClient) CleanupSecretFunc(t *testing.T, id sdk.SchemaObjectIdentifier) func() {


Nit: In general, we name these functions like DropFunc. Also, Secret should be not present in function names.

sfc-gh-jmichalak · 2024-09-19T12:19:07Z

pkg/acceptance/helpers/secret_client.go

+	return func() {
+		_, err := c.client().ShowByID(ctx, id)
+		if errors.Is(err, sdk.ErrObjectNotExistOrAuthorized) {
+			return


Cool that you thought about checking an object here, but when we use IF EXISTS, this is not needed.

sfc-gh-jmichalak · 2024-09-19T12:25:33Z

pkg/sdk/secrets_def.go

+	Field("Owner", "string").
+	Field("Comment", "string").
+	Field("SecretType", "string").
+	Field("OauthScopes", "sql.NullString").


In general, we try to parse fields containing lists/objects in convert, so that they are available as slices in the object struct. So, this should be "[]string". The same applies to OauthScopes in SecretDetails.

sfc-gh-jmichalak · 2024-09-19T12:50:03Z

pkg/sdk/secrets_def.go

+	Field("OwnerRoleType", "string")
+
+var secretDetailsDbRow = g.DbStruct("secretDetailsDBRow").
+	Field("created_on", "string").


Why no Time()?

sfc-gh-jmichalak · 2024-09-19T12:55:10Z

pkg/sdk/secrets_def.go

+		SQL("SECRET").
+		IfNotExists().
+		Name().
+		PredefinedQueryStructField("Type", "string", g.StaticOptions().SQL("TYPE = PASSWORD")).


Nit: rather than having this field exported, you could use a name like secretType. In other places as well.

sfc-gh-jmichalak · 2024-09-20T09:31:25Z

pkg/sdk/testint/secrets_gen_integration_test.go

+		assert.NotEmpty(t, s.DatabaseName)
+		assert.NotEmpty(t, s.SchemaName)
+		assert.NotEmpty(t, s.OwnerRoleType)
+		assert.NotEmpty(t, s.Owner)


Now we use generated asserts - check views integration tests.

sfc-gh-jmichalak · 2024-09-20T09:36:59Z

pkg/sdk/secrets_gen.go

+	SchemaName                  string
+	DatabaseName                string
+	Owner                       string
+	Comment                     sql.NullString


These sql.* types should be present only in dbRows - we want to separate things returned from Snowflake from things returned by the SDK. You can use *string here.

sfc-gh-jmichalak · 2024-09-20T09:41:49Z

pkg/sdk/testint/secrets_gen_integration_test.go

+			Name:                        id.Name(),
+			SecretType:                  "OAUTH2",
+			Comment:                     "a",
+			OauthRefreshTokenExpiryTime: stringDateToSnowflakeTimeFormat(time.DateOnly, refreshTokenExpiryTime),


@sfc-gh-jcieslak Do we have a helper function for comparing time? If not, this could be moved to helpers somewhere.

sfc-gh-jmichalak · 2024-09-20T09:45:40Z

pkg/sdk/testint/secrets_gen_integration_test.go

+					),
+			)
+		err := client.Secrets.Alter(ctx, setRequest)
+		require.NoError(t, err)


You're missing assert here.

sfc-gh-jmichalak · 2024-09-20T09:47:28Z

pkg/sdk/testint/secrets_gen_integration_test.go

+		require.Contains(t, returnedSecrets, *secretGenericString)
+	})
+
+	t.Run("Show: SecretWithOAuthClientCredentialsFlow with Like", func(t *testing.T) {


It's cool you've provided all of these cases, but I think that one case for each filtering option is enough (with an arbitrary type).

sfc-gh-fbudzynski added 19 commits September 19, 2024 12:55

initial secret_def file for sdk

464335d

naming changes to secret create options

ce9698a

create secrets and alter with two basic tests

16da010

unit tests for alter

072b184

add tests for all four create types

3677891

rename previous secret to secretReference in common_types.go due to n…

9a668a4

…aming clash

secrets gen with drop, show and desc

0dede41

all unit tests

ea27edf

init for integration tests

68ff325

added secret to client.go and created secret_client

97b4c53

added like and in to showByID for secret

90487f3

init for integration tests

8c144e7

secret_def changes

635c9ab

tests for two methods of creating secret

f428b25

changed refresh token expiry time from string to time.Time

28f5e4f

added remaining integration tests

7d6a0f8

linter

161ec2e

linting

e6328e6

removing comment

8c5dcfb

sfc-gh-fbudzynski force-pushed the add-secret-to-sdk branch from 929c810 to 8c5dcfb Compare September 19, 2024 11:43

sfc-gh-fbudzynski requested review from sfc-gh-jcieslak and sfc-gh-jmichalak September 19, 2024 12:05

sfc-gh-fbudzynski marked this pull request as ready for review September 19, 2024 12:12

sfc-gh-jcieslak reviewed Sep 20, 2024

View reviewed changes

sfc-gh-jmichalak requested changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add secret to sdk #3091

feat: Add secret to sdk #3091

sfc-gh-fbudzynski commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-fbudzynski Sep 20, 2024 •

edited

Loading

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jcieslak Sep 20, 2024

sfc-gh-jmichalak left a comment

sfc-gh-jmichalak Sep 19, 2024

sfc-gh-jmichalak Sep 19, 2024

sfc-gh-jmichalak Sep 19, 2024

sfc-gh-jmichalak Sep 19, 2024

sfc-gh-jmichalak Sep 19, 2024

sfc-gh-jmichalak Sep 20, 2024

sfc-gh-jmichalak Sep 20, 2024

sfc-gh-jmichalak Sep 20, 2024

sfc-gh-jmichalak Sep 20, 2024

sfc-gh-jmichalak Sep 20, 2024


		integrationId := testClientHelper().Ids.RandomAccountObjectIdentifier()

		refreshTokenExpiryTime := time.Now().Add(Day).Format(time.DateOnly)

feat: Add secret to sdk #3091

Are you sure you want to change the base?

feat: Add secret to sdk #3091

Conversation

sfc-gh-fbudzynski commented Sep 19, 2024 • edited Loading

Changes

References

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-fbudzynski Sep 20, 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

sfc-gh-jmichalak left a 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-fbudzynski commented Sep 19, 2024 •

edited

Loading

sfc-gh-fbudzynski Sep 20, 2024 •

edited

Loading