fix: data retention time parameters follow-up #2530

sfc-gh-jcieslak · 2024-02-20T13:13:31Z

A follow-up for #2502.
In this pr I applied the same changes to the Database and Table resource. On the note side, I left some comments in the table resource. There's still a deprecated data retention parameter that I would remove because it wouldn't be used with the new implementation that always expects a value (-1 if not set). We can add a note in the migration note and change the description of the deprecated parameter stating that it doesn't have any effect on the table configuration. Also in the database tests I'm changing account-level data retention time which I think shouldn't (but may) affect other tests. I would leave it anyway, because on delete we set back the default value and crashes may occur, but very rarely (I think).

Test Plan

acceptance tests

github-actions · 2024-02-20T13:37:14Z

Integration tests failure for 46c33fe1882fac50129c4918dfccecce52c947ed

github-actions · 2024-02-20T14:01:59Z

Integration tests failure for 69932795972b96a700090e3814b35e462e7a4685

github-actions · 2024-02-20T14:32:47Z

Integration tests failure for 9dac7b3116b0e3cd07cf93008b5626cb7710497f

# Conflicts: # pkg/resources/table_acceptance_test.go

github-actions · 2024-02-21T11:51:42Z

Integration tests cancelled for c003b8614ca11411bf3c6aa01776e265908e7c93

docs/resources/table.md

pkg/resources/testdata/TestAcc_Database_DefaultDataRetentionTime/WithDatabase/test.tf

sfc-gh-asawicki · 2024-02-22T12:53:42Z

pkg/resources/helpers_test.go

@@ -548,3 +548,23 @@ func queriedPrivilegesContainAtLeast(query func(client *sdk.Client, ctx context.
 		return nil
 	}
 }
+
+func updateAccountParameter(t *testing.T, client *sdk.Client, parameter sdk.AccountParameter, temporarily bool, newValue string) func() {


I don't like that this function puts so many cleanup's on the stack. There is much bigger change that one of them will fail. We base it also on the LIFO behavior of the t.Cleanup (which may never change but can).

As I wrote to you earlier, I think that the updateAccountParameterTemporarily is sufficient here. The difference is, that you should invoke the t.Cleanup just in the first step modifying the param and ignore it in the subsequent steps, i.e.:

PreConfig: func() { revertParameter := updateAccountParameterTemporarily(t, client, sdk.AccountParameterDataRetentionTimeInDays, "1") t.Cleanup(revertParameter) }

PreConfig: func() { _ := updateAccountParameterTemporarily(t, client, sdk.AccountParameterDataRetentionTimeInDays, "1") }

I think we should limit the number of additional helper functions and not add the ones similar to the already existing ones.

I added a temporarily parameter earlier but for some reason didn't use it. I'm not sure if I should just use it or return cleanup. From one side returning cleanup would be consistent with integration tests, but on the other hand this is different kind of tests where we have a framework that expects function to be provided. Maybe just using temporarily parameter and wrapping cleanup with if would be better (?) wdyt

Okay, I meant to reuse the same method or at least copy the existing one (because it is currently not importable), so that we do not have too many methods doing the exact same thing. Please just leave a TODO comment with SNOW-936093 and add it to the description of the issue (to merge this two eventually).

pkg/resources/table_acceptance_test.go

sfc-gh-asawicki · 2024-02-27T09:12:41Z

MIGRATION_GUIDE.md

-To make `snowflake_schema.data_retention_days` truly optional field (previously it was producing plan every time when no value was set),
+### snowflake_database, snowflake_schema, and snowflake_table resource changes
+#### *(behavior change)* Database `data_retention_time_in_days` + Schema `data_retention_days` + Table `data_retention_time_in_days`
+To make data retention fields truly optional (previously they were producing plan every time when no value was set),
 we added `-1` as a possible value as set it as default. That got rid of the unexpected plans when no value is set and added possibility to use default value assigned by Snowflake (see [the data retention period](https://docs.snowflake.com/en/user-guide/data-time-travel#data-retention-period)).


should it be and set it as default instead of as set it as default.?

I also had a thought recently, that we could add a issue link to give more context to the migrations.

sfc-gh-asawicki · 2024-02-27T09:29:43Z

pkg/resources/helpers_test.go

@@ -548,3 +548,23 @@ func queriedPrivilegesContainAtLeast(query func(client *sdk.Client, ctx context.
 		return nil
 	}
 }
+
+func updateAccountParameter(t *testing.T, client *sdk.Client, parameter sdk.AccountParameter, temporarily bool, newValue string) func() {


Okay, I meant to reuse the same method or at least copy the existing one (because it is currently not importable), so that we do not have too many methods doing the exact same thing. Please just leave a TODO comment with SNOW-936093 and add it to the description of the issue (to merge this two eventually).

sfc-gh-asawicki

Let's apply requested changes in the following PR (will be easier to review).

sfc-gh-asawicki · 2024-02-27T09:44:35Z

pkg/resources/table_acceptance_test.go

+		CheckDestroy: testAccCheckTableDestroy,
+		Steps: []resource.TestStep{
+			{
+				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Table_DefaultDataRetentionTime/WithDatabaseDataRetentionSet"),


one last thought... We should check setting explicit -1 (to -1 and from -1)

# Conflicts: # pkg/resources/table_acceptance_test.go

github-actions · 2024-02-27T11:05:49Z

Integration tests failure for 3288b4e2e5abc61f885e91231c622509bd1ff4bc

Follow up for #2530 - implements requested changes. - migration guide changes (added links and wording in one place) - added todo in update account param function (also updated ticket desc) - added missing test case of setting / unsetting -1 for data retention time (detected validation was set to between[0;90] instead of between[-1;90])

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

sfc-gh-jcieslak added 2 commits February 20, 2024 14:09

wip

6993279

lint fix

46c33fe

some changes

9dac7b3

sfc-gh-jcieslak added 3 commits February 21, 2024 11:04

wip

0c7da36

wip

d31d0db

Merge branch 'main' into fix-data-retention-days

c003b86

# Conflicts: # pkg/resources/table_acceptance_test.go

sfc-gh-asawicki requested changes Feb 22, 2024

View reviewed changes

Changes after review

230ee50

sfc-gh-asawicki self-requested a review February 27, 2024 08:59

sfc-gh-asawicki reviewed Feb 27, 2024

View reviewed changes

sfc-gh-asawicki previously approved these changes Feb 27, 2024

View reviewed changes

Merge branch 'main' into fix-data-retention-days

3288b4e

# Conflicts: # pkg/resources/table_acceptance_test.go

sfc-gh-jcieslak dismissed sfc-gh-asawicki’s stale review via 3288b4e February 27, 2024 10:04

sfc-gh-asawicki approved these changes Feb 27, 2024

View reviewed changes

sfc-gh-jcieslak merged commit 5544544 into main Feb 27, 2024
8 of 9 checks passed

sfc-gh-jcieslak deleted the fix-data-retention-days branch February 27, 2024 13:24

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

chore(main): release 0.87.0 #2513

Merged

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

fix: data retention days follow up #2566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: data retention time parameters follow-up #2530

fix: data retention time parameters follow-up #2530

sfc-gh-jcieslak commented Feb 20, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 21, 2024

sfc-gh-asawicki Feb 22, 2024

sfc-gh-jcieslak Feb 26, 2024 •

edited

Loading

sfc-gh-asawicki Feb 27, 2024

sfc-gh-asawicki Feb 27, 2024

sfc-gh-asawicki Feb 27, 2024

sfc-gh-asawicki left a comment

sfc-gh-asawicki Feb 27, 2024

github-actions bot commented Feb 27, 2024

fix: data retention time parameters follow-up #2530

fix: data retention time parameters follow-up #2530

Conversation

sfc-gh-jcieslak commented Feb 20, 2024 • edited Loading

Test Plan

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 20, 2024

github-actions bot commented Feb 21, 2024

sfc-gh-asawicki Feb 22, 2024

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 27, 2024

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 27, 2024

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 27, 2024

Choose a reason for hiding this comment

sfc-gh-asawicki left a comment

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 27, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 27, 2024

sfc-gh-jcieslak commented Feb 20, 2024 •

edited

Loading

sfc-gh-jcieslak Feb 26, 2024 •

edited

Loading