Update documentation for google_bigquery_table.time_partitioning.expiration_ms by wj-chen · Pull Request #8827 · GoogleCloudPlatform/magic-modules

wj-chen · 2023-08-31T18:10:21Z

Fixes hashicorp/terraform-provider-google#15626.

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

bigquery: update documentation for google_bigquery_table.time_partitioning.expiration_ms

…ry_table

modular-magician · 2023-08-31T18:11:08Z

Hello! I am a robot. It looks like you are a: ~~Community Contributor~~ Googler ~~Core Contributor~~. Tests will run automatically.

@c2thorn, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

modular-magician · 2023-08-31T18:19:47Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field time_partitioning.expiration_ms transitioned from optional+computed to optional google_bigquery_table - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 1 insertion(+), 1 deletion(-))
Terraform Beta: Diff ( 2 files changed, 1 insertion(+), 1 deletion(-))

wj-chen · 2023-08-31T18:47:22Z

Didn't realize this would be a breaking change. If it can potentially make it in to the 5.0.0 release I'll reparent this PR and prepare a corresponding doc change.

modular-magician · 2023-08-31T19:33:29Z

Tests analytics

Total tests: 3008
Passed tests 2707
Skipped tests: 296
Affected tests: 5

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryTable_YearlyTimePartitioning|TestAccBigQueryTable_HourlyTimePartitioning|TestAccBigQueryTable_MonthlyTimePartitioning|TestAccBigQueryTable_Basic|TestAccBigQueryDataTable_bigtable

Get to know how VCR tests work

modular-magician · 2023-08-31T19:43:21Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryTable_YearlyTimePartitioning[Debug log]
TestAccBigQueryTable_HourlyTimePartitioning[Debug log]
TestAccBigQueryTable_MonthlyTimePartitioning[Debug log]
TestAccBigQueryTable_Basic[Debug log]
TestAccBigQueryDataTable_bigtable[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

wj-chen · 2023-09-05T19:20:11Z

Didn't realize this would be a breaking change. If it can potentially make it in to the 5.0.0 release I'll reparent this PR and prepare a corresponding doc change.

Waiting for guidance

c2thorn · 2023-09-06T15:26:59Z

 						"expiration_ms": {
 							Type:        schema.TypeInt,
 							Optional:    true,
-							Computed:    true,


computed is usually set when the contributor has found that the API returns the value for the field in some scenario where the user has not set it. Is it confirmed that the API never returns a value for this when not set?

Reached out to the API team about it and waiting for official answer there, meanwhile exploring alternatives - if I need to keep Computed: true, should I then set a Default: nil alongside it to signal "use this default value if it's not in the config"?

Also thank you for your explanation of Computed. Much clearer than the official documentation...

Computed: true and any Default conflict with each other. Essentially Optional + Computed means "default to the returned API value". Also adding a new Default value is a breaking change.

If Computed is needed, I think we can add custom logic to allow unsetting the value. I've seen RawConfig used before: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.erb#L2436
In the expander function for this field, I think you can check the user value to see if it is nil, then return that instead. I'm not 100% confident, and it will need some testing however. RawConfig is somewhat new. Doing that would not be a breaking change.

Thank you. Meanwhile I learned why this attribute was marked Computed: true - BigQuery tables belong in BigQuery datasets, a separate resource, and a dataset may come with a default expiration time, which should be propagated to all the new tables created in it. In the case that a dataset default expiration is set, the absence of table-level expiration should be treated as "using the dataset-level default" instead of "no expiration".

Unless there is a way in the provider to achieve conditionally treating this value differently based on a dataset field value (is there?), given the nature of this design, I'm inclined to treat the current behavior as working-as-intended and just update documentation to suggest users to specify expiration_ms = 0 if they need to explicitly unset. Let me know what you think.

Unless there is a way in the provider to achieve conditionally treating this value differently based on a dataset field value (is there?)

We have scenarios where we call the read API for one resource from within another. As far as reading the "Terraform set" value from another resource, I don't think we've ever done that (although it may be possible?)

Either way, it seems that the field should reasonably stay Computed.

I'm inclined to treat the current behavior as working-as-intended and just update documentation to suggest users to specify expiration_ms = 0 if they need to explicitly unset. Let me know what you think.

That seems reasonable to me. I should have checked if expiration_ms = 0 worked earlier, that is in line with other similar fields that are Computed.

We have scenarios where we call the read API for one resource from within another.

Thank you, this should be enough in this case, but I'll first reply in the GitHub issue explaining the situation with Computed and pursuing the WAI path first, unless there is strong push back there. Will comment here again once the PR is updated accordingly.

We have scenarios where we call the read API for one resource from within another.

Thank you, this should be enough in this case, but I'll first reply in the GitHub issue explaining the situation with Computed and pursuing the WAI path first, unless there is strong push back there. Will comment here again once the PR is updated accordingly.

User confirmed in hashicorp/terraform-provider-google#15626 that leaving the field Computed: true and setting the value to 0 achieved the desired effect. I updated the PR to be only a documentation change (and a trivial test change).

And thank you for all the insights in this thread!

c2thorn · 2023-09-06T15:28:21Z

Didn't realize this would be a breaking change. If it can potentially make it in to the 5.0.0 release I'll reparent this PR and prepare a corresponding doc change.

Waiting for guidance

Asked a question in another comment. If moving forward, then yes this can go into 5.0.0

…ration_ms

modular-magician · 2023-09-15T19:16:22Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

Field time_partitioning.expiration_ms transitioned from optional+computed to optional google_bigquery_table - reference

If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An override-breaking-change label can be added to allow merging.

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 6 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 2 files changed, 6 insertions(+), 3 deletions(-))

modular-magician · 2023-09-15T19:28:54Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 2 files changed, 6 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 6 insertions(+), 2 deletions(-))

modular-magician · 2023-09-15T20:40:30Z

Tests analytics

Total tests: 3055
Passed tests 2756
Skipped tests: 298
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataSourceGoogleServiceAccountIdToken_impersonation

Get to know how VCR tests work

modular-magician · 2023-09-15T20:42:52Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

…ration_ms (GoogleCloudPlatform#8827)

allow users to unset time_partitioning.expiration_ms in google_bigque…

cc4c5eb

…ry_table

modular-magician requested a review from c2thorn August 31, 2023 18:11

c2thorn reviewed Sep 6, 2023

View reviewed changes

wj-chen added 2 commits September 15, 2023 12:05

Merge branch 'GoogleCloudPlatform:main' into issue-15626

b53373a

update documentation for google_bigquery_table.time_partitioning.expi…

45eca52

…ration_ms

modular-magician requested a review from c2thorn September 15, 2023 19:07

wj-chen changed the title ~~Allow users to unset time_partitioning.expiration_ms in google_bigquery_table~~ Update documentation for google_bigquery_table.time_partitioning.expiration_ms Sep 15, 2023

add Computed:true back to expirationMs

4bda69c

c2thorn approved these changes Sep 15, 2023

View reviewed changes

c2thorn merged commit 7d9dc66 into GoogleCloudPlatform:main Sep 15, 2023

This was referenced Sep 15, 2023

Update documentation for google_bigquery_table.time_partitioning.expiration_ms hashicorp/terraform-provider-google-beta#6290

Merged

Update documentation for google_bigquery_table.time_partitioning.expiration_ms hashicorp/terraform-provider-google#15873

Merged

joelkattapuram pushed a commit to joelkattapuram/magic-modules that referenced this pull request Sep 20, 2023

Update documentation for google_bigquery_table.time_partitioning.expi…

e391ffb

…ration_ms (GoogleCloudPlatform#8827)

nevzheng pushed a commit to nevzheng/magic-modules that referenced this pull request Sep 22, 2023

Update documentation for google_bigquery_table.time_partitioning.expi…

bd50b5d

…ration_ms (GoogleCloudPlatform#8827)

Uh oh!

Conversation

wj-chen commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

modular-magician commented Aug 31, 2023

Uh oh!

modular-magician commented Aug 31, 2023

Breaking Change(s) Detected

Diff report

Uh oh!

wj-chen commented Aug 31, 2023

Uh oh!

modular-magician commented Aug 31, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Aug 31, 2023

Rerun these tests in REPLAYING mode to catch issues

Uh oh!

wj-chen commented Sep 5, 2023

Uh oh!

c2thorn Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

wj-chen Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

c2thorn Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wj-chen Sep 9, 2023

Choose a reason for hiding this comment

Uh oh!

c2thorn Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

wj-chen Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

wj-chen Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c2thorn commented Sep 6, 2023

Uh oh!

modular-magician commented Sep 15, 2023

Breaking Change(s) Detected

Diff report

Uh oh!

modular-magician commented Sep 15, 2023

Diff report

Uh oh!

modular-magician commented Sep 15, 2023

Tests analytics

Action taken

Uh oh!

modular-magician commented Sep 15, 2023

Rerun these tests in REPLAYING mode to catch issues

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wj-chen commented Aug 31, 2023 •

edited

Loading

c2thorn Sep 6, 2023 •

edited

Loading

wj-chen Sep 15, 2023 •

edited

Loading