-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add redis_configs parameter to Redis Cluster #10515
Conversation
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. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_redis_cluster" "primary" {
redis_configs = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccRedisCluster_redisClusterHaExample|TestAccRedisCluster_updateRedisConfigs|TestAccRedisCluster_updateReplicaCount |
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. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_redis_cluster" "primary" {
redis_configs = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccRedisCluster_redisClusterHaExample|TestAccRedisCluster_updateRedisConfigs|TestAccRedisCluster_updateReplicaCount |
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -9,6 +9,9 @@ resource "google_redis_cluster" "<%= ctx[:primary_resource_id] %>" { | |||
node_type = "REDIS_SHARED_CORE_NANO" | |||
transit_encryption_mode = "TRANSIT_ENCRYPTION_MODE_DISABLED" | |||
authorization_mode = "AUTH_MODE_DISABLED" | |||
redis_configs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redis_configs { | |
redis_configs = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good catch! I think this is the problem. Let me fix it now.
type ClusterParams struct { | ||
name string | ||
replicaCount int | ||
shardCount int | ||
preventDestroy bool | ||
nodeType string | ||
redisConfigs map[string]string | ||
} | ||
|
||
func createOrUpdateRedisCluster(params *ClusterParams) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice improvement, thanks!
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccRedisCluster_redisClusterHaExample |
|
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:
I am not sure why the presubmit doesn't capture this issue though. |
I can't reproduce this currently. Sometimes I've seen similar issues when I generate the providers using the When you generate the provider using I think is the first error you shared is showing where it says |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add support of RedisConfigs for Memorystore Redis Cluster
Github issue: hashicorp/terraform-provider-google#17941