Support Delayed Deletion for Vmwareengine Private Cloud #10764

swamitagupta · 2024-05-22T06:57:04Z

Adds support to delay the Private Cloud deletion . More information about the intended behavior can be found here.
Modifies the region for vmwareengine resource tests to region 'me-west1' with allocated quota.
Facilitates type change from TIME_LIMITED to STANDARD for multi-node Private Clouds

Release Note Template for Downstream PRs (will be copied)

vmwareengine: added `deletion_delay_hours` field to `google_vmwareengine_private_cloud` resource

vmwareengine: support type change from `TIME_LIMITED` to `STANDARD` for multi-node `google_vmwareengine_private_cloud` resource

github-actions · 2024-05-22T06:57:40Z

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

@melinath, 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 · 2024-05-22T07:08:07Z

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 ( 3 files changed, 59 insertions(+), 46 deletions(-))
google-beta provider: Diff ( 3 files changed, 59 insertions(+), 46 deletions(-))

modular-magician · 2024-05-22T07:12:24Z

Tests analytics

Total tests: 16
Passed tests: 11
Skipped tests: 2
Affected tests: 3

Click here to see the affected service packages

vmwareengine

Action taken

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

TestAccVmwareengineExternalAccessRule_vmwareEngineExternalAccessRuleUpdate|TestAccVmwareengineExternalAddress_vmwareEngineExternalAddressUpdate|TestAccVmwareengineSubnet_vmwareEngineUserDefinedSubnetUpdate

Get to know how VCR tests work

swamitagupta · 2024-05-22T07:20:32Z

This PR intends to solve #10764

modular-magician · 2024-05-22T08:51:33Z

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 ( 3 files changed, 59 insertions(+), 46 deletions(-))
google-beta provider: Diff ( 3 files changed, 59 insertions(+), 46 deletions(-))

modular-magician · 2024-05-22T08:56:16Z

Tests analytics

Total tests: 16
Passed tests: 11
Skipped tests: 2
Affected tests: 3

Click here to see the affected service packages

vmwareengine

Action taken

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

TestAccVmwareengineExternalAccessRule_vmwareEngineExternalAccessRuleUpdate|TestAccVmwareengineExternalAddress_vmwareEngineExternalAddressUpdate|TestAccVmwareengineSubnet_vmwareEngineUserDefinedSubnetUpdate

Get to know how VCR tests work

modular-magician · 2024-05-22T09:49:53Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccVmwareengineExternalAccessRule_vmwareEngineExternalAccessRuleUpdate[Error message] [Debug log]
TestAccVmwareengineExternalAddress_vmwareEngineExternalAddressUpdate[Error message] [Debug log]
TestAccVmwareengineSubnet_vmwareEngineUserDefinedSubnetUpdate[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

modular-magician · 2024-05-22T12:56:31Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccVmwareengineExternalAccessRule_vmwareEngineExternalAccessRuleUpdate[Error message] [Debug log]
TestAccVmwareengineSubnet_vmwareEngineUserDefinedSubnetUpdate[Error message] [Debug log]

$\textcolor{red}{\textsf{Several tests got terminated during RECORDING mode.}}$
$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

github-actions · 2024-05-23T09:39:48Z

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

swamitagupta · 2024-05-23T12:28:23Z

Hi @melinath,
Wanted to let you know that I am in the process of testing various CUJs locally and will update you once the changes have successfully passed the tests and are ready to be reviewed for submission.

melinath

ah okay - in that case, marking as reviewed for now.

github-actions · 2024-06-06T09:40:32Z

@swamitagupta, this PR is waiting for action from you. Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

If no action is taken, this PR will be closed in 28 days.

This notification can be disabled with the disable-automatic-closure label.

modular-magician · 2024-07-03T05:27:48Z

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

vmwareengine

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$

View the build log

modular-magician · 2024-07-03T05:28:31Z

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 ( 6 files changed, 152 insertions(+), 54 deletions(-))
google-beta provider: Diff ( 6 files changed, 152 insertions(+), 54 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 29 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.

modular-magician · 2024-07-03T05:30:28Z

Tests analytics

Total tests: 0
Passed tests: 0
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

vmwareengine

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR.}}$

View the build log

modular-magician · 2024-07-03T05:37:42Z

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 ( 6 files changed, 148 insertions(+), 54 deletions(-))
google-beta provider: Diff ( 6 files changed, 148 insertions(+), 54 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 29 insertions(+), 1 deletion(-))

modular-magician · 2024-07-03T05:41:35Z

Tests analytics

Total tests: 16
Passed tests: 11
Skipped tests: 5
Affected tests: 0

Click here to see the affected service packages

vmwareengine

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

modular-magician · 2024-07-03T05:49:08Z

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 ( 6 files changed, 148 insertions(+), 54 deletions(-))
google-beta provider: Diff ( 6 files changed, 148 insertions(+), 54 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 29 insertions(+), 1 deletion(-))

modular-magician · 2024-07-03T05:52:54Z

Tests analytics

Total tests: 16
Passed tests: 11
Skipped tests: 5
Affected tests: 0

Click here to see the affected service packages

vmwareengine

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

modular-magician · 2024-07-03T08:40:52Z

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 ( 6 files changed, 148 insertions(+), 54 deletions(-))
google-beta provider: Diff ( 6 files changed, 148 insertions(+), 54 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 29 insertions(+), 1 deletion(-))

modular-magician · 2024-07-03T08:45:18Z

Tests analytics

Total tests: 16
Passed tests: 11
Skipped tests: 5
Affected tests: 0

Click here to see the affected service packages

vmwareengine

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log

github-actions · 2024-07-05T09:41:06Z

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

github-actions · 2024-07-09T09:41:27Z

@GoogleCloudPlatform/terraform-team @c2thorn @melinath This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

c2thorn

asked some clarifying questions for context, things seem okay at first glance however.
Lots of parts, but since changes are contained to a diff suppress or deletion functionality, it should be fairly safe.

c2thorn · 2024-07-09T13:36:41Z

mmv1/products/vmwareengine/PrivateCloud.yaml

+  - !ruby/object:Api::Type::Integer
+    name: "deletion_delay_hours"
+    description: |
+      The number of hours to delay this request. You can set this value to an hour between 0 to 8, where setting it to 0 starts the deletion request immediately. If no value is set, a default value is set at the API Level.


What is the API default value for deletion delay? Is it 0? How often do you think users will need to additionally set send_deletion_delay_hours_if_zero?

Currently, the default delay at the API level is set to 3 hours. However, it might be modified in the near future.

Users would need to set send_deletion_delay_hours_if_zero if they want to send delay_hours=0 through the API call and delete the PC immediately. I have added this boolean field after a discussion with Stephen in a previous thread since terraform does not differentiate between "no value set" and "0 value set".

Okay, thank you for the context.

Just noting that I do not like the need for send_deletion_delay_hours_if_zero but cannot think of another way to force send 0 only when the user has specified 0 due to TF limitations. I am sure this was discussed previously

Yes, this is the previous discussion thread for implementing this change.

Another approach which I could think of was using delay_hours inside a nested object deletion_parameters but nested objects are not yet supported for virtual_fields.

c2thorn · 2024-07-09T13:42:29Z

mmv1/templates/terraform/constants/vmwareengine_private_cloud.go.erb

+		return true
+	}
+    if (isMultiNodePrivateCloud(d) && old == "TIME_LIMITED" && new == "STANDARD") {
+        log.Printf("[DEBUG] Multinode Private Cloud found, facilitating TYPE change to STANDARD")


Can you give more context as to why a type diff from TIME_LIMITED -> STANDARD needs to be suppressed with Multinode? Also, is this related or separate to delayed deletion?

This change is separate from delayed deletion.

The TIME_LIMITED -> STANDARD PC update behavior is explained in go/upgrade-pc-in-terraform

A feature request was raised where the user wanted to change the PC type from their end for this use case. Since API does not support this change in its update call (it automatically changes it in the backend after a node count 1 -> 3&+ node count increase), I have added this suppress function to enable this type change at the terraform level.

I also see that type is set to ignore_read, so at least it will remain consistent. Thanks for the context.

c2thorn · 2024-07-09T13:44:41Z

mmv1/third_party/terraform/services/vmwareengine/resource_vmwareengine_private_cloud_test.go

@@ -18,7 +18,7 @@ func TestAccVmwareenginePrivateCloud_vmwareEnginePrivateCloudUpdate(t *testing.T
 	t.Parallel()

 	context := map[string]interface{}{
-		"region":          "southamerica-west1",
+		"region":          "me-west1",


can you provide more context as to why this is being updated?

Some months back, we exhausted the number of nodes in the southamerca-west1 region which were being used for this test, leading to PC creation failures in multiple terraform tests.

As a result, go/gcve-resource-optimisation-for-tf-tests was created and additional nodes were reserved in me-west1 region for terraform test projects. This PR is part of its implementation efforts along with PR #10992.

c2thorn · 2024-07-09T13:45:23Z

mmv1/third_party/terraform/services/vmwareengine/resource_vmwareengine_subnet_test.go

@@ -8,6 +8,8 @@ import (
 )

 func TestAccVmwareengineSubnet_vmwareEngineUserDefinedSubnetUpdate(t *testing.T) {
+	// Temporarily skipping so that this test does not run and consume resources during PR pushes. It is bound to fail and is being fixed by PR #10992


we will remove these before submitting right?

We can keep these commented till PR #10992 gets submitted. That way, the consumption of nodes for tests which are bound to fail would be prevented.

These tests would be modified and combined by PR #10992 which is built on the design go/gcve-resource-optimisation-for-tf-tests

Understood, so the relevant test for these code changes are all in mmv1/third_party/terraform/services/vmwareengine/resource_vmwareengine_private_cloud_test.go

Yes, thats correct

c2thorn

LGTM

stale

…latform#10764)

github-actions bot requested a review from melinath May 22, 2024 06:57

modular-magician added the service/vmwareengine label May 22, 2024

swamitagupta force-pushed the pc-delayed-deletion branch from bed5a73 to aabdf56 Compare May 22, 2024 08:39

melinath reviewed May 23, 2024

View reviewed changes

swamitagupta force-pushed the pc-delayed-deletion branch from aabdf56 to 9a502e9 Compare June 14, 2024 05:20

github-actions bot requested a review from melinath June 14, 2024 05:21

This comment was marked as outdated.

Sign in to view

swamitagupta force-pushed the pc-delayed-deletion branch 2 times, most recently from e104f8e to 89c8f23 Compare June 14, 2024 06:23

This comment was marked as outdated.

Sign in to view

swamitagupta force-pushed the pc-delayed-deletion branch from 89c8f23 to a8b6b8c Compare June 14, 2024 08:00

This comment was marked as outdated.

Sign in to view

swamitagupta force-pushed the pc-delayed-deletion branch from a8b6b8c to bf09ae3 Compare June 14, 2024 09:36

swamitagupta marked this pull request as ready for review July 3, 2024 05:35

github-actions bot requested a review from melinath July 3, 2024 05:36

swamitagupta force-pushed the pc-delayed-deletion branch from 2939d07 to 1d01423 Compare July 3, 2024 05:38

Add Delayed Deletion to Vmwareengine Private Cloud

bb90373

swamitagupta force-pushed the pc-delayed-deletion branch from 1d01423 to bb90373 Compare July 3, 2024 08:29

c2thorn reviewed Jul 9, 2024

View reviewed changes

c2thorn approved these changes Jul 10, 2024

View reviewed changes

c2thorn removed the request for review from melinath July 10, 2024 17:41

c2thorn merged commit b8a463e into GoogleCloudPlatform:main Jul 10, 2024
15 of 16 checks passed

pcostell pushed a commit to pcostell/magic-modules that referenced this pull request Jul 16, 2024

Support Delayed Deletion for Vmwareengine Private Cloud (GoogleCloudP…

32eebe8

…latform#10764)

vijaykanthm pushed a commit to vijaykanthm/magic-modules that referenced this pull request Jul 22, 2024

Support Delayed Deletion for Vmwareengine Private Cloud (GoogleCloudP…

943f975

…latform#10764)

SarahFrench mentioned this pull request Jul 30, 2024

Ignore deletion_delay_hours when importing during TestAccVmwareenginePrivateCloud_vmwareEnginePrivateCloudUpdate #11295

Merged

Charlesleonius pushed a commit to Charlesleonius/magic-modules that referenced this pull request Aug 1, 2024

Support Delayed Deletion for Vmwareengine Private Cloud (GoogleCloudP…

250c995

…latform#10764)

Support Delayed Deletion for Vmwareengine Private Cloud #10764

Support Delayed Deletion for Vmwareengine Private Cloud #10764

Conversation

swamitagupta commented May 22, 2024 • edited by c2thorn Loading

github-actions bot commented May 22, 2024

modular-magician commented May 22, 2024

Diff report

modular-magician commented May 22, 2024

Tests analytics

Action taken

swamitagupta commented May 22, 2024

modular-magician commented May 22, 2024

Diff report

modular-magician commented May 22, 2024

Tests analytics

Action taken

modular-magician commented May 22, 2024

modular-magician commented May 22, 2024

github-actions bot commented May 23, 2024

swamitagupta commented May 23, 2024

melinath left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2024

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

modular-magician commented Jul 3, 2024

Tests analytics

modular-magician commented Jul 3, 2024

Diff report

Errors

modular-magician commented Jul 3, 2024

Tests analytics

modular-magician commented Jul 3, 2024

Diff report

modular-magician commented Jul 3, 2024

Tests analytics

modular-magician commented Jul 3, 2024

Diff report

modular-magician commented Jul 3, 2024

Tests analytics

modular-magician commented Jul 3, 2024

Diff report

modular-magician commented Jul 3, 2024

Tests analytics

github-actions bot commented Jul 5, 2024

github-actions bot commented Jul 9, 2024

c2thorn left a 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

c2thorn Jul 10, 2024 • edited Loading

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

c2thorn Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c2thorn left a comment

Choose a reason for hiding this comment

swamitagupta commented May 22, 2024 •

edited by c2thorn

Loading

c2thorn Jul 10, 2024 •

edited

Loading

c2thorn Jul 10, 2024 •

edited

Loading