Add redis_configs parameter to Redis Cluster

[liuce-]

Add support of RedisConfigs for Memorystore Redis Cluster

Github issue: hashicorp/terraform-provider-google#17941

redis: added `redis_configs` field to `google_redis_cluster` resource

[github-actions]

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

@SarahFrench, 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]

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, 54 insertions(+))
google-beta provider: Diff ( 4 files changed, 157 insertions(+), 35 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 3 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_redis_cluster (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_redis_cluster" "primary" {
  redis_configs = # value needed
}

[modular-magician]

Tests analytics

Total tests: 17
Passed tests: 14
Skipped tests: 0
Affected tests: 3

Click here to see the affected service packages

• redis

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

TestAccRedisCluster_redisClusterHaExample|TestAccRedisCluster_updateRedisConfigs|TestAccRedisCluster_updateReplicaCount

Get to know how VCR tests work

[liuce-]

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, 54 insertions(+)) google-beta provider: Diff ( 4 files changed, 157 insertions(+), 35 deletions(-)) terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+)) Open in Cloud Shell: Diff ( 1 file changed, 3 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_redis_cluster (1 total tests) Please add an acceptance test which includes these fields. The test should include the following:

resource "google_redis_cluster" "primary" {
  redis_configs = # value needed
}

I am not sure why it complains that my PR doesn't include tests for this new field. I added test for both create & update tests.

[modular-magician]

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, 54 insertions(+))
google-beta provider: Diff ( 4 files changed, 157 insertions(+), 35 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 3 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_redis_cluster (1 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_redis_cluster" "primary" {
  redis_configs = # value needed
}

[modular-magician]

Tests analytics

Total tests: 17
Passed tests: 14
Skipped tests: 0
Affected tests: 3

Click here to see the affected service packages

• redis

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

TestAccRedisCluster_redisClusterHaExample|TestAccRedisCluster_updateRedisConfigs|TestAccRedisCluster_updateReplicaCount

Get to know how VCR tests work

[modular-magician]

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

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

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccRedisCluster_redisClusterHaExample[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]

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

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

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccRedisCluster_redisClusterHaExample[Error message] [Debug log]
TestAccRedisCluster_updateRedisConfigs[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

[SarahFrench]

Thanks for your PR! My review includes a fix for one of the current test failures and hopefully that'll address the feedback about test coverage.

The TestAccRedisCluster_updateRedisConfigs test is currently failing due to the error below reason, and I suspect the other test might fail the same way once the HCL syntax is changed:

  "error": {
    "code": 400,
    "message": "Cannot delete ServiceConnectionPolicy projects/PROJECT/locations/us-central1/serviceConnectionPolicies/tf-test-7867583993478641267 because it still has 1 PSC Connections associated with it: failed precondition",
    "status": "FAILED_PRECONDITION"
  }

Can you think of any changes to the test to help address this? Sometimes when we test deletion protection fields on resources we have to include an additional step at the end of tests to remove that protection, so that the test can clean up resources successfully. Is there something similar that could be implemented here?

[SarahFrench]

Suggested change

	redis_configs {
	redis_configs = {

[SarahFrench]

Currently this syntax is treating this field like a block, but as the field is a map of arbitrary string key:value pairs it needs this =, like you've done elsewhere in your PR. Making this change will address the current problem in the TestAccRedisCluster_redisClusterHaExample test.

I think this might be the cause of the missing test detector saying the field isn't being tested correctly. We can see what the tool says once a change is pushed 😁

[liuce-]

yeah good catch! I think this is the problem. Let me fix it now.

[SarahFrench]

This is a nice improvement, thanks!

[modular-magician]

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, 54 insertions(+))
google-beta provider: Diff ( 4 files changed, 157 insertions(+), 35 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 17 insertions(+))
Open in Cloud Shell: Diff ( 1 file changed, 3 insertions(+))

[modular-magician]

Tests analytics

Total tests: 17
Passed tests: 16
Skipped tests: 0
Affected tests: 1

Click here to see the affected service packages

• redis

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

TestAccRedisCluster_redisClusterHaExample

Get to know how VCR tests work

[modular-magician]

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

$\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

[liuce-]

All tests passed now.

I do want to call out that it seems there is an issue with the current codebase (in main branch) that blocks developers from running tests manually.

For example, when I built the magic-module or ran tests (even in main branch), I got the following errors:

==> Checking that code complies with gofmt requirements...
go vet
# github.com/hashicorp/terraform-provider-google/google/provider
google/provider/provider_mmv1_resources.go:598:78: undefined: compute.ResourceComputeInstanceSettings
google/provider/provider_mmv1_resources.go:637:78: undefined: compute.ResourceComputeSecurityPolicyRule
google/provider/provider_mmv1_resources.go:690:89: undefined: datalossprevention.ResourceDataLossPreventionDiscoveryConfig
google/provider/provider_mmv1_resources.go:861:83: undefined: integrations.ResourceIntegrationsAuthConfig
google/provider/provider_mmv1_resources.go:896:90: undefined: networkconnectivity.ResourceNetworkConnectivityInternalRange
make: *** [GNUmakefile:29: vet] Error 1
==> Checking that code complies with gofmt requirements...
go vet
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/parallelstore: no non-test Go files in /usr/local/google/home/calvinliu/go/src/github.com/hashicorp/terraform-provider-google-beta/google-beta/services/parallelstore
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/privilegedaccessmanager: no non-test Go files in /usr/local/google/home/calvinliu/go/src/github.com/hashicorp/terraform-provider-google-beta/google-beta/services/privilegedaccessmanager
make: *** [GNUmakefile:29: vet] Error 1

I am not sure why the presubmit doesn't capture this issue though.

[SarahFrench]

I do want to call out that it seems there is an issue with the current codebase (in main branch) that blocks developers from running tests manually.

I can't reproduce this currently. Sometimes I've seen similar issues when I generate the providers using the PRODUCT variable shown in the docs and I've also got old versions of the hashicorp/terraform-provider-google(-beta) repos' code locally.

When you generate the provider using PRODUCT it will only generate the code for the packages describing those products/services and the 'central' provider code that connects all those service packages together. This means that if the generator is run with PRODUCT=redis and the repo at OUTPUT_PATH is 6 months out of date it will update the redis package and the 'central' provider code but all other packages will be 6 months behind, and the 'central' provider code may reference code that has been added to those other packages in those 6 months. I hope that makes sense!

I think is the first error you shared is showing where it says undefined, and I think that could be the reason for the other errors too. Try to solve the problem you're seeing by 1) ensure that your hashicorp/terraform-provider-google(-beta) repos have main branch checked out and latest changes pulled down, and 2) have a go omitting the PRODUCT variable when generating the provider.

[SarahFrench]

Approved - after the latest changes everything looks good! I ran all the redis acceptance tests against the GA and Beta providers to double check and they pass without evidence of the issue where the test fails to clean up resources after itself.