feat: Add support for table column comments and to control a tables data retention and change tracking settings #614

robbruce · 2021-07-22T12:43:30Z

Adds the following options to the table resource

comments on columns
property to control DATA_RETENTION_TIME_IN_DAYS on a table
property to control CHANGE_TRACKING on a table

Test Plan

acceptance tests

$ TF_ACC=1 go test -v ./... -run TestAcc_Table
?   	github.com/chanzuckerberg/terraform-provider-snowflake	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/datasources	(cached) [no tests to run]
?   	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/db	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/provider	(cached) [no tests to run]
=== RUN   TestAcc_Table
=== PAUSE TestAcc_Table
=== CONT  TestAcc_Table
--- PASS: TestAcc_Table (57.92s)
PASS
ok  	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/resources	58.328s
testing: warning: no tests to run
PASS
ok  	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/snowflake	(cached) [no tests to run]
?   	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/testhelpers	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/validation	(cached) [no tests to run]
?   	github.com/chanzuckerberg/terraform-provider-snowflake/pkg/version	[no test files]

References

None

…ble data retention

berosen · 2021-07-22T23:49:14Z

pkg/resources/table.go

+	"data_retention_days": {
+		Type:         schema.TypeInt,
+		Optional:     true,
+		Default:      1,


Will this default value overwrite the inherited value from a higher level object? I believe the default behavior in snowflake is to inherit from the "higher object". For example, if I were to set data_retention_days=20 for my schema, I'd expect that to be inherited by the table. This seems like it might break that?

I also think since the variable is called data_retention_time_in_days we should also name it that here. https://docs.snowflake.com/en/sql-reference/parameters.html#data-retention-time-in-days

It does look like the Default value is 1 in the docs though.

More context
If a retention period is specified for a database or schema, the period is inherited by default for all objects created in the database/schema

from https://docs.snowflake.com/en/user-guide/data-time-travel.html

@alldoami @berosen - I really wasn't sure what to use as snowflake_schema has data_retention_days and snowflake_database has data_retention_time_in_days so there's inconsistencies already.

https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/main/pkg/resources/schema.go#L54
https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/main/pkg/resources/database.go#L25

@berosen - yes you're right it will override the default from a parent object, I could change this so that if its nil then the setting is not applied to the table if that's more suitable. Which in the case of using this provider, it will be the schema as that has the logic I have here, e.g. it defaults to 1, rather than defaults to inherit.

@alldoami - I can change the behaviour of the snowflake_schema resource too, but obviously that will have some side effects for existing deployments. I have no idea why on the database the value is computed though.

@robbruce , personally I like the idea of using nil. I feel the change would be more in-line with expected behaviour. I think making the change in snowflake_schema would be a good idea as well. However, I'll defer to whatever @alldoami feels is best.

@berosen - I have been looking into this, and I'm not sure its possible with how the current typing system works for schema.TypeInt - see hashicorp/terraform-plugin-sdk#261

Essentially , if I set Default to be nil, then the SDK returns this as 0 and so there doesn't appear to be a nice way to inherit.

In addition, when reading a table with SHOW TABLES, then even if the value was not supplied (so it inherits) then it returns the value defined in it's parent schema, so even then there is no way to detect if there is a difference.

Ah ok I see your reasoning now with the naming @robbruce.

@robbruce That's a good point, I think the work around for that would be complicated. Maybe it's enough to write a note in the description that the inheritance needs to be explicitly defined by passing the same value as the parent object? ie

snowflake_table foo { ... data_retention_days=snowflake_schema.name. data_retention_days }

Sorry for the noise all!

@berosen @alldoami - updated the description as suggested and the table example too.

alldoami · 2021-07-22T23:54:37Z

/ok-to-test sha=b75a091

github-actions · 2021-07-23T00:00:24Z

Integration tests success for b75a091

tracking table options

alldoami · 2021-07-23T19:34:08Z

/ok-to-test sha=5eb437d

github-actions · 2021-07-23T19:40:21Z

Integration tests failure for 5eb437d

… updated docs

alldoami · 2021-07-26T17:26:55Z

/ok-to-test sha=5d770a1

github-actions · 2021-07-26T17:35:19Z

Integration tests failure for 5d770a1

robbruce · 2021-07-26T19:25:22Z

@alldoami - what needs to be done to sort out the integration-trusted test?

alldoami · 2021-07-26T21:49:13Z

@robbruce can you add change_tracking to True to https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/main/pkg/resources/stream_grant_acceptance_test.go#L78-L91

…hancements

robbruce · 2021-07-27T08:30:09Z

@robbruce can you add change_tracking to True to https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/main/pkg/resources/stream_grant_acceptance_test.go#L78-L91

@alldoami - done, and rebased with main

alldoami · 2021-07-27T17:46:34Z

/ok-to-test sha=05e036d

github-actions · 2021-07-27T17:52:58Z

Integration tests failure for 05e036d

alldoami · 2021-07-27T18:00:52Z

Need to do the same here: https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/39631932d6e90e4707a73cca9c5f1237cf3c3a1c/pkg/resources/stream_acceptance_test.go#L47-L60

robbruce · 2021-07-27T20:02:24Z

Need to do the same here:

https://github.com/chanzuckerberg/terraform-provider-snowflake/blob/39631932d6e90e4707a73cca9c5f1237cf3c3a1c/pkg/resources/stream_acceptance_test.go#L47-L60

@alldoami - all done

alldoami · 2021-07-28T00:17:07Z

/ok-to-test sha=f2adee7

github-actions · 2021-07-28T00:24:12Z

Integration tests success for f2adee7

alldoami

Woo! Nice

…ata retention and change tracking settings (Snowflake-Labs#614)

robbruce added 2 commits July 22, 2021 13:32

Added support for table column comments, table change tracking and ta…

36d434f

…ble data retention

Updated docs for table resource

b75a091

robbruce requested a review from a team as a code owner July 22, 2021 12:43

robbruce requested a review from alldoami July 22, 2021 12:43

robbruce changed the title ~~Add support for table column comments and to control a tables data retention and change tracking settings~~ feat: Add support for table column comments and to control a tables data retention and change tracking settings Jul 22, 2021

berosen reviewed Jul 22, 2021

View reviewed changes

Added missing changes for reading and creating data retention and change

5eb437d

tracking table options

Updated description for snowflake_table.data_retention_days argument,…

5d770a1

… updated docs

robbruce added 2 commits July 27, 2021 09:25

Merge remote-tracking branch 'original/main' into f/table_resource_en…

4754a21

…hancements

Added change_tracking = true to TestAccStreamGrant acceptance test

05e036d

Added change_tracking to TestAcc_Stream tests

f2adee7

alldoami approved these changes Jul 28, 2021

View reviewed changes

alldoami merged commit daa46a0 into Snowflake-Labs:main Jul 28, 2021

robbruce deleted the f/table_resource_enhancements branch July 28, 2021 07:46

jtzero pushed a commit to rxrevu/terraform-provider-snowflake that referenced this pull request Aug 19, 2021

feat: Add support for table column comments and to control a tables d…

a5dc7e0

…ata retention and change tracking settings (Snowflake-Labs#614)

anton-chekanov pushed a commit to anton-chekanov/terraform-provider-snowflake that referenced this pull request Jan 25, 2022

feat: Add support for table column comments and to control a tables d…

cb14601

…ata retention and change tracking settings (Snowflake-Labs#614)

daniepett pushed a commit to daniepett/terraform-provider-snowflake that referenced this pull request Feb 9, 2022

feat: Add support for table column comments and to control a tables d…

27d50d3

…ata retention and change tracking settings (Snowflake-Labs#614)

snowflake-release-please bot mentioned this pull request May 25, 2022

chore(main): release 0.34.0 #1022

Merged

This was referenced Nov 1, 2022

chore(main): release 0.34.0 #1329

Closed

chore(main): release 0.34.0 #1330

Closed

chore(main): release 0.49.0 #1332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for table column comments and to control a tables data retention and change tracking settings #614

feat: Add support for table column comments and to control a tables data retention and change tracking settings #614

robbruce commented Jul 22, 2021

berosen Jul 22, 2021

alldoami Jul 22, 2021

berosen Jul 23, 2021

robbruce Jul 23, 2021

berosen Jul 23, 2021

robbruce Jul 23, 2021

alldoami Jul 23, 2021

berosen Jul 23, 2021

robbruce Jul 24, 2021

alldoami commented Jul 22, 2021

github-actions bot commented Jul 23, 2021

alldoami commented Jul 23, 2021

github-actions bot commented Jul 23, 2021

alldoami commented Jul 26, 2021

github-actions bot commented Jul 26, 2021

robbruce commented Jul 26, 2021

alldoami commented Jul 26, 2021 •

edited

Loading

robbruce commented Jul 27, 2021

alldoami commented Jul 27, 2021

github-actions bot commented Jul 27, 2021

alldoami commented Jul 27, 2021

robbruce commented Jul 27, 2021

alldoami commented Jul 28, 2021

github-actions bot commented Jul 28, 2021

alldoami left a comment

feat: Add support for table column comments and to control a tables data retention and change tracking settings #614

feat: Add support for table column comments and to control a tables data retention and change tracking settings #614

Conversation

robbruce commented Jul 22, 2021

Test Plan

References

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

alldoami commented Jul 22, 2021

github-actions bot commented Jul 23, 2021

alldoami commented Jul 23, 2021

github-actions bot commented Jul 23, 2021

alldoami commented Jul 26, 2021

github-actions bot commented Jul 26, 2021

robbruce commented Jul 26, 2021

alldoami commented Jul 26, 2021 • edited Loading

robbruce commented Jul 27, 2021

alldoami commented Jul 27, 2021

github-actions bot commented Jul 27, 2021

alldoami commented Jul 27, 2021

robbruce commented Jul 27, 2021

alldoami commented Jul 28, 2021

github-actions bot commented Jul 28, 2021

alldoami left a comment

Choose a reason for hiding this comment

alldoami commented Jul 26, 2021 •

edited

Loading