chore: Remove unused old implementation from snowflake pkg #2458

sfc-gh-asawicki · 2024-02-02T17:07:46Z

Remove unused code from snowflake package
Migrate datasources missed earlier
Migrate small statements still using old package

github-actions · 2024-02-02T17:21:03Z

Integration tests failure for 887f5e1466cf856b8c878490f6a8376c1d942e61

github-actions · 2024-02-05T15:02:20Z

Integration tests failure for 004372c111fbec2037bb6649fa8d1f0f5259b0ff

github-actions · 2024-02-05T16:08:27Z

Integration tests failure for 004372c111fbec2037bb6649fa8d1f0f5259b0ff

github-actions · 2024-02-05T16:58:20Z

Integration tests failure for ce142dcaa4ab0ae6155e86e8d30aa73bd728e28b

sfc-gh-swinkler · 2024-02-05T22:19:59Z

pkg/datasources/resource_monitors.go


-	account, err := snowflake.ReadCurrentAccount(db)
+	account, err := client.ContextFunctions.Current(ctx)


i think this function should be named CurrentAccount() https://docs.snowflake.com/en/sql-reference/functions/current_account. Also should this be put in a helper struct so it can be something like client.Context.Sessions.CurrentAccount()? Or maybe just client.ContextSessions.CurrentAccount?

We already have current account check another comment).

I don't understand the second part, though. It's already exposed in client's ContextFunctions interface.

I don't have a proposal for the name other than client.ContextFunctions.SessionContext(). Until we come up with a better name I don't mind staying with Current as only we are the users of the SDK.

Renamed to CurrentSessionDetails (because CurrentSession was already taken too).

sfc-gh-swinkler · 2024-02-05T22:23:27Z

pkg/datasources/resource_monitors_acceptance_test.go

+		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
+		PreCheck:                 func() { acc.TestAccPreCheck(t) },
+		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
+			tfversion.RequireAbove(tfversion.Version1_5_0),


eventually we should make this a configurable variable that can be set in one place and used everywhere. will allow us to upgrade required tf version as necessary

sfc-gh-swinkler · 2024-02-05T22:25:30Z

pkg/datasources/users.go

 	userPattern := d.Get("pattern").(string)

-	account, err := snowflake.ReadCurrentAccount(db)
-	if err != nil {
+	account, err1 := client.ContextFunctions.CurrentAccount(ctx)


wait we already have CurrentAccount? Then what was the previous Current()? I don't see that anywhere in here: https://docs.snowflake.com/en/sql-reference/functions-context

The problem is, we've already had a family of CurrentX() functions. In the old snowflake package there was one method, fetching both account and region. To spare two invocation, I created a new one in the SDK, which fetches all these CurrentX params. We can name it differently if you have an idea how. CurrentAll? CurrentAccountAll? any ideas? cc: @sfc-gh-jcieslak

if your concern is with multiple invocations, there is a way we can fix that by batching requests together. I don't know exactly how we would accomplish this, but it is worth thinking about.

It was not a concern about the multiple invocations performance but the need to check the errors for each. Now, we fetch multiple values together, and I think it's easier to use it.

sfc-gh-swinkler · 2024-02-05T22:28:13Z

pkg/resources/user_public_keys.go

@@ -60,10 +62,12 @@ func UserPublicKeys() *schema.Resource {
 }

 func checkUserExists(db *sql.DB, name string) (bool, error) {


this seems like a rather pointless function, we should probably remove this entirely. But not important right now

I am not sure, but I tried not to modify the existing logic in this PR.

sfc-gh-swinkler · 2024-02-05T22:31:08Z

pkg/sdk/context_functions.go

@@ -130,6 +159,21 @@ func (c *contextFunctions) CurrentUser(ctx context.Context) (string, error) {
 	return s.CurrentUser, nil
 }

+func (c *contextFunctions) Current(ctx context.Context) (*CurrentDetails, error) {


okay, i see now. Is this really necessary though? just because it was done in a similar way in the old snowflake package, doesn't mean we need to do it again here. also there are more current functions than just what is listed here.

There are multiple times when we want to fetch both account and region, for me it is better to fetch all at once and not shoot for every single param.

sfc-gh-jcieslak · 2024-02-06T09:50:21Z

pkg/datasources/resource_monitors.go


-	account, err := snowflake.ReadCurrentAccount(db)
+	account, err := client.ContextFunctions.Current(ctx)


I don't have a proposal for the name other than client.ContextFunctions.SessionContext(). Until we come up with a better name I don't mind staying with Current as only we are the users of the SDK.

github-actions · 2024-02-06T11:25:15Z

Integration tests failure for 89294266c45e1fb3a07c2f0587b2ffadf55d8fbb

🤖 I have created a release *beep* *boop* --- # Release notes [0.86.0](v0.85.0...v0.86.0) (2024-02-15) ## 🎉 **What's new** * add refresh_mode and initialize to dynamic tables ([#2437](#2437)) ([d301b20](d301b20)) * add resource snowflake_user_password_policy_attachment ([#2162](#2162)) ([#2307](#2307)) ([93af462](93af462)) * create a workaround for granting privileges on all pipes ([#2477](#2477)) ([64f2346](64f2346)) * Handle IMPORTED PRIVILEGES privileges in privilege-to-role granting resources ([#2471](#2471)) ([eb20051](eb20051)) * use external functions ([#2454](#2454)) ([417d473](417d473)) * use funcs from sdk ([#2462](#2462)) ([a5f969c](a5f969c)) * use sdk for procedures ([#2450](#2450)) ([94ac78a](94ac78a)) * Use sdk in table constraint resource ([#2466](#2466)) ([d685603](d685603)) * Use tables from SDK ([#2453](#2453)) ([fdb4f88](fdb4f88)) ## 🔧 **Misc** * Add migration notes to the docs and change jira integration ([#2497](#2497)) ([b17f1af](b17f1af)) * Change email and issue reporter ([#2470](#2470)) ([5865655](5865655)) * Grants migration guide ([#2455](#2455)) ([62c70fd](62c70fd)) * Remove unused old implementation from snowflake pkg ([#2458](#2458)) ([2d0e508](2d0e508)) * update password policy attachment ([#2485](#2485)) ([6ec9ff7](6ec9ff7)) ## 🐛 **Bug fixes** * allow DT warehouse to be updated in-place ([#2439](#2439)) ([d565af1](d565af1)) * correct test dependencies ([#2493](#2493)) ([dfb247f](dfb247f)) * FileFormat not detecting changes correctly ([#2436](#2436)) ([018bb74](018bb74)) * Fix few smaller issues ([#2507](#2507)) ([a836871](a836871)) * Fix functions and small other fixes ([#2503](#2503)) ([0d4aba4](0d4aba4)), closes [#2490](#2490) * Fix tag tests in view and in materialized view ([#2457](#2457)) ([2de942a](2de942a)) * Fix task related issues ([#2479](#2479)) ([0385650](0385650)) * Fix tests that base on default data retention ([#2465](#2465)) ([682e28c](682e28c)) * grant privileges to share test terraform dependencies ([#2473](#2473)) ([ede8d95](ede8d95)) * parameter issues ([#2463](#2463)) ([7ee4986](7ee4986)) * parse dynamic table query from DDL ([#2438](#2438)) ([d76815c](d76815c)) * Remove title and body temporarily from jira integration ([#2499](#2499)) ([672c97d](672c97d)) * SHOW GRANTS mapping for share data type ([#2508](#2508)) ([feb4d44](feb4d44)) * user error handling ([#2486](#2486)) ([dfa52b2](dfa52b2)) --- 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 8 commits February 2, 2024 18:05

Remove user from snowflake package

7660802

Remove current user from snowflake package

3596e94

Migrate current account datasource

01ec3a4

Migrate resource monitor datasource

508a019

Migrate role datasource

4b5d373

Remove current account logic from snowflake package

32c0376

Remove unused logic from snowflake package

87b27c0

Fix linter complaints

887f5e1

sfc-gh-asawicki changed the title ~~chore: Remove unused old implementation~~ chore: Remove unused old implementation from snowflake pkg Feb 2, 2024

Fix network policy

004372c

sfc-gh-asawicki marked this pull request as ready for review February 5, 2024 16:16

sfc-gh-asawicki requested review from sfc-gh-jcieslak and sfc-gh-swinkler February 5, 2024 16:17

Add issue number to TODO comment

ce142dc

sfc-gh-swinkler reviewed Feb 5, 2024

View reviewed changes

sfc-gh-jcieslak previously approved these changes Feb 6, 2024

View reviewed changes

sfc-gh-asawicki added 2 commits February 6, 2024 11:36

Fix after review

c445254

Merge branch 'main' into remove-unused-old-implementation

8929426

sfc-gh-asawicki dismissed sfc-gh-jcieslak’s stale review via 8929426 February 6, 2024 10:43

sfc-gh-asawicki requested review from sfc-gh-jcieslak and sfc-gh-swinkler February 6, 2024 10:43

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

View reviewed changes

sfc-gh-asawicki merged commit 2d0e508 into main Feb 6, 2024
8 of 9 checks passed

sfc-gh-asawicki deleted the remove-unused-old-implementation branch February 6, 2024 11:35

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

chore(main): release 0.86.0 #2456

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Remove unused old implementation from snowflake pkg #2458

chore: Remove unused old implementation from snowflake pkg #2458

sfc-gh-asawicki commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

sfc-gh-swinkler Feb 5, 2024

sfc-gh-asawicki Feb 6, 2024

sfc-gh-jcieslak Feb 6, 2024

sfc-gh-asawicki Feb 6, 2024

sfc-gh-swinkler Feb 5, 2024

sfc-gh-swinkler Feb 5, 2024

sfc-gh-asawicki Feb 6, 2024

sfc-gh-swinkler Feb 6, 2024

sfc-gh-asawicki Feb 6, 2024

sfc-gh-swinkler Feb 5, 2024

sfc-gh-asawicki Feb 6, 2024

sfc-gh-swinkler Feb 5, 2024

sfc-gh-asawicki Feb 6, 2024

sfc-gh-jcieslak Feb 6, 2024

github-actions bot commented Feb 6, 2024


		account, err := snowflake.ReadCurrentAccount(db)
		account, err := client.ContextFunctions.Current(ctx)

		@@ -60,10 +62,12 @@ func UserPublicKeys() *schema.Resource {
		}

		func checkUserExists(db *sql.DB, name string) (bool, error) {

chore: Remove unused old implementation from snowflake pkg #2458

chore: Remove unused old implementation from snowflake pkg #2458

Conversation

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

github-actions bot commented Feb 2, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

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 Feb 6, 2024

sfc-gh-asawicki commented Feb 2, 2024 •

edited

Loading