AB #10674

ma-g-22 · 2024-05-12T21:30:59Z

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)

feat: add bigtable automated backup configuration

ron-gal · 2024-05-13T13:43:20Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go

+		}
+		tblConf.AutomatedBackupConfig = &bigtable.TableAutomatedBackupPolicy{
+			RetentionPeriod: abpRetentionPeriod,
+			Frequency:       abpFrequency}


this looks like weird formatting. I'd have abpFrequency, and } on a new line

Thanks, updated

ron-gal · 2024-05-13T13:44:05Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go

+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: verify.ValidateDuration(),
+							Description:  `How long the automated backups should be retained. The only supported value at this time is 3 days.`,


That's a server side validation, right? We should remember to edit this text when we change the server side validation

Thanks. That's right. To simplify future updates, I've removed the reference to particular values here

ron-gal · 2024-05-13T13:44:13Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go

+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: verify.ValidateDuration(),
+							Description:  `How frequently automated backups should occur. The only supported value at this time is 24 hours.`,


same as above

Thanks. That's right. To simplify future updates, I've removed the reference to particular values here

ron-gal · 2024-05-13T13:46:03Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go

+						"frequency": {
+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: verify.ValidateDuration(),


how does the validation work?
For garbage collection policy, we created a new~ ParseDuration function that can handled weeks (I think only weeks? maybe more). Do we want to use that one instead of the time.ParseDuration? If so, I'm not sure what ValidateDuration does, but maybe we want to do the same for the validation (it'd be weird if it didn't pass validation, but was valid everywhere else...)

Thanks

I'm reusing the same verify.ValidateDuration that change_stream_retention above uses (which also parses using time.ParseDuration) https://github.com/ma-g-22/magic-modules/blob/ab/mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go#L103

Is this the new ParseDuration function? https://github.com/ma-g-22/magic-modules/blob/ab/mmv1/third_party/terraform/services/bigtable/duration_parsing_helper.go#L26 - I could change it to use this, though I'm not sure if we need weeks specifically?

That's the difference between these two. I am not sure how critical it is for your use case, but there is nothing to lose by moving to it (I am pretty sure of that, at least. Best to make sure with some manual\automatic tests)

TracyCuiCan · 2024-05-15T18:03:18Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go

+							Type:         schema.TypeString,
+							Optional:     true,
+							ValidateFunc: verify.ValidateDuration(),
+							Description:  `How long the automated backups should be retained. The only supported value at this time is 3 days.`,


Shall we also mention that "set to 0 to disable"? Same thing for frequency.

Discussed offline, per https://docs.google.com/document/d/1NzFf9Fi3MLJFG4sZgeVhbxMlxbKumF7xPQ1U3mgquJs we are not doing 0 to disable.

Thanks. On further thought, I believe we should continue with 0-to-disable

I've updated the code here

I've also posted a PR here: googleapis/google-cloud-go#10226

TracyCuiCan · 2024-05-15T18:13:41Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table.go

@@ -335,6 +401,36 @@ func resourceBigtableTableUpdate(d *schema.ResourceData, meta interface{}) error
 		}
 	}

+	if d.HasChange("automated_backup_policy") {
+		abpField := d.Get("automated_backup_policy")
+		if abpField == nil {


I probably got confused about what we decided, I thought we want to follow CDC and disable when retention_period and frequency is set as 0, and no change if automated_backup_policy is null.

Discussed offline, this is implementing the right behavior defined in https://docs.google.com/document/d/1NzFf9Fi3MLJFG4sZgeVhbxMlxbKumF7xPQ1U3mgquJs, i.e. nill to disable.

Thanks. On further thought, I believe we should continue with 0-to-disable

I've updated the code here

I've also posted a PR here: googleapis/google-cloud-go#10226

TracyCuiCan · 2024-05-15T18:16:39Z

mmv1/third_party/terraform/services/bigtable/resource_bigtable_table_test.go

@@ -418,6 +489,35 @@ func testAccBigtableChangeStreamDisabled(t *testing.T) resource.TestCheckFunc {
 	}
 }

+func testAccBigtableAutomatedBackupsDisabled(t *testing.T) resource.TestCheckFunc {


test or Test? Seems inconsistent?

Thanks. I've renamed this to verifyBigtableAutomatedBackupsDisabled to reduce confusion. This is not a test per-se, but a verification function used as part of the TestAccBigtableTable_automated_backups_enable test

github-actions · 2024-05-21T16:15:22Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@trodge, 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.

github-actions · 2024-05-23T09:40:03Z

This PR has been waiting for review for 2 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

modular-magician · 2024-05-23T20:53:09Z

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.

google provider: Diff ( 4 files changed, 292 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 4 files changed, 292 insertions(+), 1 deletion(-))

Errors

google provider:

The diff processor failed to build. This is usually due to the downstream provider failing to compile.

google-beta provider:

The diff processor failed to build. This is usually due to the downstream provider failing to compile.

trodge · 2024-05-24T18:32:05Z

/gcbrun

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 12, 2024

ron-gal reviewed May 13, 2024

View reviewed changes

TracyCuiCan reviewed May 15, 2024

View reviewed changes

ma-g-22 closed this May 21, 2024

ma-g-22 force-pushed the ab branch from 48bcaec to 6461ee1 Compare May 21, 2024 13:59

ma-g-22 reopened this May 21, 2024

feat: add bigtable automated backup configuration

a0e06b0

ma-g-22 force-pushed the ab branch from 19d2ea0 to a0e06b0 Compare May 21, 2024 15:57

ma-g-22 marked this pull request as ready for review May 21, 2024 16:14

github-actions bot requested a review from trodge May 21, 2024 16:15

modular-magician added service/bigtableadmin and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels May 23, 2024

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AB #10674

AB #10674

ma-g-22 commented May 12, 2024 •

edited

ron-gal May 13, 2024

ma-g-22 May 21, 2024

ron-gal May 13, 2024

ma-g-22 May 21, 2024

ron-gal May 13, 2024

ma-g-22 May 21, 2024

ron-gal May 13, 2024

ma-g-22 May 21, 2024 •

edited

ron-gal May 24, 2024

TracyCuiCan May 15, 2024

TracyCuiCan May 15, 2024

ma-g-22 May 21, 2024

TracyCuiCan May 15, 2024

TracyCuiCan May 15, 2024

ma-g-22 May 21, 2024

TracyCuiCan May 15, 2024

ma-g-22 May 21, 2024

github-actions bot commented May 21, 2024

github-actions bot commented May 23, 2024

modular-magician commented May 23, 2024

trodge commented May 24, 2024

AB #10674

Are you sure you want to change the base?

AB #10674

Conversation

ma-g-22 commented May 12, 2024 • edited

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

ma-g-22 May 21, 2024 • edited

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 May 21, 2024

github-actions bot commented May 23, 2024

modular-magician commented May 23, 2024

Diff report

Errors

trodge commented May 24, 2024

ma-g-22 commented May 12, 2024 •

edited

ma-g-22 May 21, 2024 •

edited