specs out the additional fields for nmve_ssd #7427

geojaz · 2023-03-09T03:37:13Z

Adds local_nvme_ssd_block to node_config block in the google_container_node_pool.

Also I needed to update the go.sum and go.mod and wasn't really clear on the right process.
Does this look right? Is there a "right" way to do it? Mine was really hacky :)

Closes hashicorp/terraform-provider-google#13829

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 the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

container: added `local_nvme_ssd_block` to `node_config` block in the `google_container_node_pool`.

shuyama1 · 2023-03-10T22:28:22Z

/gcbrun

modular-magician · 2023-03-10T22:48:26Z

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 ( 3 files changed, 334 insertions(+), 84 deletions(-))
Terraform Beta: Diff ( 5 files changed, 444 insertions(+), 111 deletions(-))
TF Validator: Diff ( 2 files changed, 59 insertions(+), 49 deletions(-))

modular-magician · 2023-03-10T22:50:10Z

Tests analytics

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

Errors occurred during REPLAYING mode. Please fix them to complete your PR
View the build log

geojaz · 2023-03-11T00:15:31Z

I can't see the logs for the failures, but i think it might have something to do with updating the deps for google.golang.org/api v0.111.0.
I'll have to take a closer look to the tests on my side unless you can share some additional details- feel free to ping me on gchat @Ehole
(Just kidding, it was a bad test name)

shuyama1 · 2023-03-13T17:17:37Z

/gcbrun

modular-magician · 2023-03-13T17:32: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 ( 3 files changed, 335 insertions(+), 85 deletions(-))
Terraform Beta: Diff ( 5 files changed, 445 insertions(+), 112 deletions(-))
TF Validator: Diff ( 2 files changed, 63 insertions(+), 53 deletions(-))

modular-magician · 2023-03-13T17:34:07Z

Tests analytics

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

Errors occurred during REPLAYING mode. Please fix them to complete your PR
View the build log

shuyama1 · 2023-03-13T18:00:09Z

Tests failed due to:

go: bitbucket.org/creachadair/stringset@v0.0.8: missing go.sum entry; to add it:
	go mod download bitbucket.org/creachadair/stringset
make: *** [vet] Error 1
GNUmakefile:26: recipe for target 'vet' failed
make: *** No rule to make target 'tools'.  Stop.
==> Checking that code complies with gofmt requirements...
go vet
go: bitbucket.org/creachadair/stringset@v0.0.8: missing go.sum entry; to add it:
	go mod download bitbucket.org/creachadair/stringset
make: *** [vet] Error 1
GNUmakefile:26: recipe for target 'vet' failed
==> Checking that code complies with gofmt requirements...
go vet
go: bitbucket.org/creachadair/stringset@v0.0.8: missing go.sum entry; to add it:
	go mod download bitbucket.org/creachadair/stringset
make: *** [vet] Error 1

shuyama1 · 2023-03-13T19:30:14Z

/gcbrun

modular-magician · 2023-03-13T19:45:49Z

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 ( 3 files changed, 356 insertions(+), 35 deletions(-))
Terraform Beta: Diff ( 5 files changed, 466 insertions(+), 62 deletions(-))
TF Validator: Diff ( 2 files changed, 63 insertions(+), 53 deletions(-))

modular-magician · 2023-03-13T20:32:16Z

Tests analytics

Total tests: 2497
Passed tests 2218
Skipped tests: 262
Affected tests: 17

Action taken

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

TestAccPrivatecaCaPool_updateCaOption|TestAccPrivatecaCertificate_privatecaCertificateUpdate|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccPrivatecaCertificate_privatecaCertificateCsrExample|TestAccPrivatecaCertificate_privatecaCertificateConfigExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityUpdate|TestAccPrivatecaCertificateAuthority_rootCaManageDesiredState|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample|TestAccPrivatecaCertificate_privatecaCertificateWithTemplateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerNodePool_localNvmeSsdBlockConfig|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline|TestAccPrivatecaCaPool_privatecaCapoolUpdate|TestAccPrivatecaCaPool_privatecaCapoolAllFieldsExample|TestAccDataSourcePrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample|TestAccDataSourceDnsManagedZone_basic

Get to know how VCR tests work

modular-magician · 2023-03-13T21:02:10Z

Tests passed during RECORDING mode:
TestAccPrivatecaCaPool_updateCaOption[Debug log]
TestAccPrivatecaCertificate_privatecaCertificateUpdate[Debug log]
TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample[Debug log]
TestAccPrivatecaCertificate_privatecaCertificateCsrExample[Debug log]
TestAccPrivatecaCertificate_privatecaCertificateConfigExample[Debug log]
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityUpdate[Debug log]
TestAccPrivatecaCertificateAuthority_rootCaManageDesiredState[Debug log]
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample[Debug log]
TestAccPrivatecaCertificate_privatecaCertificateWithTemplateExample[Debug log]
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComposerEnvironment_withEncryptionConfigComposer2[Debug log]
TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline[Debug log]
TestAccPrivatecaCaPool_privatecaCapoolUpdate[Debug log]
TestAccPrivatecaCaPool_privatecaCapoolAllFieldsExample[Debug log]
TestAccDataSourcePrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample[Debug log]

Tests failed during RECORDING mode:
TestAccContainerNodePool_localNvmeSsdBlockConfig[Error message] [Debug log]
TestAccDataSourceDnsManagedZone_basic[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

geojaz · 2023-03-13T23:17:25Z

/gcbrun

shuyama1 · 2023-03-13T23:21:27Z

/gcbrun

modular-magician · 2023-03-13T23:36:53Z

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 ( 3 files changed, 357 insertions(+), 35 deletions(-))
Terraform Beta: Diff ( 5 files changed, 475 insertions(+), 62 deletions(-))
TF Validator: Diff ( 2 files changed, 63 insertions(+), 53 deletions(-))

modular-magician · 2023-03-14T00:23:11Z

Tests analytics

Total tests: 2497
Passed tests 2231
Skipped tests: 262
Affected tests: 4

Action taken

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

TestAccComputeForwardingRule_update|TestAccFirebaserulesRelease_BasicRelease|TestAccContainerNodePool_localNvmeSsdBlockConfig|TestAccDataSourceDnsManagedZone_basic

Get to know how VCR tests work

modular-magician · 2023-03-14T00:24:38Z

Tests passed during RECORDING mode:
TestAccComputeForwardingRule_update[Debug log]
TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode:
TestAccContainerNodePool_localNvmeSsdBlockConfig[Error message] [Debug log]
TestAccDataSourceDnsManagedZone_basic[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1 · 2023-03-14T05:30:45Z

/gcbrun

shuyama1 · 2023-03-14T06:21:11Z

/gcbrun

modular-magician · 2023-03-14T06:36:58Z

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 ( 3 files changed, 357 insertions(+), 35 deletions(-))
Terraform Beta: Diff ( 5 files changed, 474 insertions(+), 62 deletions(-))
TF Validator: Diff ( 2 files changed, 63 insertions(+), 53 deletions(-))

modular-magician · 2023-03-14T07:22:15Z

Tests analytics

Total tests: 2497
Passed tests 2232
Skipped tests: 262
Affected tests: 3

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

TestAccFirebaserulesRelease_BasicRelease|TestAccContainerNodePool_localNvmeSsdBlockConfig|TestAccDataSourceDnsManagedZone_basic

Get to know how VCR tests work

modular-magician · 2023-03-14T07:34:11Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccContainerNodePool_localNvmeSsdBlockConfig[Debug log]

Tests failed during RECORDING mode:
TestAccDataSourceDnsManagedZone_basic[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1

LGTM! only some small comments. Plus, any reason we add in beta provider exclusively? I do see this field is supported in V1 API though.

shuyama1 · 2023-03-14T17:15:13Z

mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown

+```hcl
+local_nvme_ssd_block_config {
+  local_ssd_count = 2
+}
+```


We don't usually include inline example for fields. If you think the configuration example is needed for users, please add a full configuration example in the example usage section. However, for this feature, I think the usage is pretty straightforward, we can simply omit the code example.

shuyama1 · 2023-03-14T17:23:24Z

mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown

@@ -878,6 +886,11 @@ linux_node_config {

 * `local_ssd_count` (Required) - Number of local SSDs to use to back ephemeral storage. Uses NVMe interfaces. Each local SSD is 375 GB in size. If zero, it means to disable using local SSDs as ephemeral storage.

+<a name="nested_local_nvme_ssd_block_config"></a>The `local_nvme_ssd_block_config` block supports:
+
+* `local_ssd_count` (Required) - Number of local NVMe SSDs attached to the instance. Each NVMe SSD is 375 GB in size.


Any particular reason that we don't align with the API doc here?

geojaz · 2023-03-14T19:52:11Z

LGTM! only some small comments. Plus, any reason we add in beta provider exclusively? I do see this field is supported in V1 API though.
Good call- I'll push it to GA. I effectively just copied the implementation of ephemeral_storage_config- looks like this should also graduate... I won't include that here.

geojaz · 2023-03-14T23:40:13Z

Pushed with your suggestions, I couldn't get the tests on generated GA provider to run on my system- there were lots of seemingly unrelated errors. Not sure what's up

samos123 · 2023-03-15T04:00:36Z

/gcbrun

shuyama1 · 2023-03-15T04:08:41Z

/gcbrun

modular-magician · 2023-03-15T04:24: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.

Terraform GA: Diff ( 5 files changed, 460 insertions(+), 60 deletions(-))
Terraform Beta: Diff ( 5 files changed, 468 insertions(+), 62 deletions(-))
TF Validator: Diff ( 2 files changed, 63 insertions(+), 53 deletions(-))

modular-magician · 2023-03-15T05:09:09Z

Tests analytics

Total tests: 2500
Passed tests 2233
Skipped tests: 262
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

TestAccFirebaserulesRelease_BasicRelease|TestAccComputeForwardingRule_update|TestAccApigeeAddonsConfig_apigeeAddonsTestExample|TestAccDataSourceDnsManagedZone_basic|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample

Get to know how VCR tests work

modular-magician · 2023-03-15T06:00:46Z

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]
TestAccComputeForwardingRule_update[Debug log]
TestAccApigeeAddonsConfig_apigeeAddonsTestExample[Debug log]
TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample[Debug log]

Tests failed during RECORDING mode:
TestAccDataSourceDnsManagedZone_basic[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1

GA test failed on TC: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstream/383054
It's probably due to one of the beta flags not removed

mmv1/third_party/terraform/utils/node_config.go.erb

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

shuyama1 · 2023-03-15T16:57:43Z

/gcbrun

modular-magician · 2023-03-15T17:13: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.

Terraform GA: Diff ( 5 files changed, 467 insertions(+), 60 deletions(-))
Terraform Beta: Diff ( 5 files changed, 468 insertions(+), 62 deletions(-))
TF Validator: Diff ( 2 files changed, 63 insertions(+), 53 deletions(-))

modular-magician · 2023-03-15T18:01:17Z

Tests analytics

Total tests: 2501
Passed tests 2237
Skipped tests: 263
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

TestAccDataSourceDnsManagedZone_basic

Get to know how VCR tests work

modular-magician · 2023-03-15T18:01:39Z

Tests failed during RECORDING mode:
TestAccDataSourceDnsManagedZone_basic[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

shuyama1 · 2023-03-15T20:42:22Z

GA test passed: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstream/383064

shuyama1

LGTM! Thank you!

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

geojaz added 2 commits March 8, 2023 19:14

specs out the additional fields for nmve_ssd

7ebfe52

Updates go.mod/go.sum for nvme_ssd feature

012186c

geojaz requested a review from a team as a code owner March 9, 2023 03:37

geojaz requested review from shuyama1 and removed request for a team March 9, 2023 03:37

Fixes name of test and updates go.mod

ef70006

geojaz force-pushed the add_nvme_ssd_to_container_node_pool branch from 22211bd to ef70006 Compare March 13, 2023 06:06

Trying to get these deps sorted

a894979

geojaz force-pushed the add_nvme_ssd_to_container_node_pool branch from 96cf496 to 2c0afee Compare March 13, 2023 23:39

geojaz force-pushed the add_nvme_ssd_to_container_node_pool branch from 2c0afee to f6eff4f Compare March 14, 2023 05:28

Fixes gke version for test and docs

0015600

geojaz force-pushed the add_nvme_ssd_to_container_node_pool branch from f6eff4f to 0015600 Compare March 14, 2023 06:14

shuyama1 reviewed Mar 14, 2023

View reviewed changes

Switches from beta to ga

6726aa8

modular-magician requested a review from shuyama1 March 15, 2023 04:09

shuyama1 requested changes Mar 15, 2023

View reviewed changes

mmv1/third_party/terraform/utils/node_config.go.erb Outdated Show resolved Hide resolved

Update mmv1/third_party/terraform/utils/node_config.go.erb

3bfc8ba

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

modular-magician requested a review from shuyama1 March 15, 2023 16:58

shuyama1 approved these changes Mar 15, 2023

View reviewed changes

shuyama1 merged commit 768b706 into GoogleCloudPlatform:main Mar 15, 2023

This was referenced Mar 15, 2023

specs out the additional fields for nmve_ssd hashicorp/terraform-provider-google-beta#5335

Merged

specs out the additional fields for nmve_ssd hashicorp/terraform-provider-google#14008

Merged

dannawang0221 pushed a commit to dannawang0221/magic-modules that referenced this pull request Mar 24, 2023

specs out the additional fields for nmve_ssd (GoogleCloudPlatform#7427)

7aa7822

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

dannawang0221 pushed a commit to dannawang0221/magic-modules that referenced this pull request Mar 24, 2023

specs out the additional fields for nmve_ssd (GoogleCloudPlatform#7427)

68c636b

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

dannawang0221 pushed a commit to dannawang0221/magic-modules that referenced this pull request Mar 29, 2023

specs out the additional fields for nmve_ssd (GoogleCloudPlatform#7427)

510c7f2

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

ericayyliu pushed a commit to ericayyliu/magic-modules that referenced this pull request Jul 26, 2023

specs out the additional fields for nmve_ssd (GoogleCloudPlatform#7427)

cdf3bca

Co-authored-by: Shuya Ma <87669292+shuyama1@users.noreply.github.com>

specs out the additional fields for nmve_ssd #7427

specs out the additional fields for nmve_ssd #7427

Conversation

geojaz commented Mar 9, 2023 • edited by shuyama1 Loading

shuyama1 commented Mar 10, 2023

modular-magician commented Mar 10, 2023

Diff report

modular-magician commented Mar 10, 2023

Tests analytics

geojaz commented Mar 11, 2023 • edited Loading

shuyama1 commented Mar 13, 2023

modular-magician commented Mar 13, 2023

Diff report

modular-magician commented Mar 13, 2023

Tests analytics

shuyama1 commented Mar 13, 2023

shuyama1 commented Mar 13, 2023

modular-magician commented Mar 13, 2023

Diff report

modular-magician commented Mar 13, 2023

Tests analytics

Action taken

modular-magician commented Mar 13, 2023

geojaz commented Mar 13, 2023

shuyama1 commented Mar 13, 2023

modular-magician commented Mar 13, 2023

Diff report

modular-magician commented Mar 14, 2023

Tests analytics

Action taken

modular-magician commented Mar 14, 2023

shuyama1 commented Mar 14, 2023

shuyama1 commented Mar 14, 2023

modular-magician commented Mar 14, 2023

Diff report

modular-magician commented Mar 14, 2023

Tests analytics

Action taken

modular-magician commented Mar 14, 2023

shuyama1 left a comment

Choose a reason for hiding this comment

shuyama1 Mar 14, 2023

Choose a reason for hiding this comment

shuyama1 Mar 14, 2023

Choose a reason for hiding this comment

geojaz commented Mar 14, 2023

geojaz commented Mar 14, 2023

samos123 commented Mar 15, 2023

shuyama1 commented Mar 15, 2023

modular-magician commented Mar 15, 2023

Diff report

modular-magician commented Mar 15, 2023

Tests analytics

Action taken

modular-magician commented Mar 15, 2023

shuyama1 left a comment

Choose a reason for hiding this comment

shuyama1 commented Mar 15, 2023

modular-magician commented Mar 15, 2023

Diff report

modular-magician commented Mar 15, 2023

Tests analytics

Action taken

modular-magician commented Mar 15, 2023

shuyama1 commented Mar 15, 2023

shuyama1 left a comment • edited Loading

Choose a reason for hiding this comment

geojaz commented Mar 9, 2023 •

edited by shuyama1

Loading

geojaz commented Mar 11, 2023 •

edited

Loading

shuyama1 left a comment •

edited

Loading