-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Redis MRR Support #5449
Redis MRR Support #5449
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in /~https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @melinath, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 84 insertions(+), 1 deletion(-)) |
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.
Please add a release note as described in the opening comment's template
mmv1/products/redis/terraform.yaml
Outdated
properties: | ||
alternativeLocationId: !ruby/object:Overrides::Terraform::PropertyOverride | ||
default_from_api: true | ||
authorizedNetwork: !ruby/object:Overrides::Terraform::PropertyOverride | ||
default_from_api: true | ||
custom_expand: 'templates/terraform/custom_expand/redis_instance_authorized_network.erb' | ||
diff_suppress_func: 'compareSelfLinkOrResourceName' | ||
locationId: !ruby/object:Overrides::Terraform::PropertyOverride |
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 seems like an extraneous change
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.
added back
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in /~https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
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.
In addition to the changes below, please update your release note per the guidelines and sign the CLA as described here: /~https://github.com/GoogleCloudPlatform/magic-modules/pull/5449/checks?check_run_id=4196015153
mmv1/products/redis/api.yaml
Outdated
name: readReplicasMode | ||
description: | | ||
Optional. Read replica mode. | ||
If not set, Memorystore Redis backend will pick the mode based on other fields in the request. |
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.
It looks from the docs like the default is always READ_REPLICAS_DISABLED - is that not the case?
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.
Updated the description.
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in /~https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
/gcbrun |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccRedisInstanceDatasource_basic|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccEventarcTrigger_BasicHandWritten|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccRedisInstance_redisInstanceMrrExample|TestAccRedisInstance_redisInstanceBasicExample|TestAccRedisInstance_update|TestAccRedisInstance_redisInstanceFullExample|TestAccRedisInstance_redisInstanceAuthEnabled|TestAccRedisInstance_regionFromLocation|TestAccRedisInstance_downgradeRedisVersion|TestAccServiceDirectoryEndpoint_serviceDirectoryEndpointWithNetworkExample|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=220806 |
Tests failed during RECORDING mode: TestAccRedisInstance_redisInstanceMrrExample|TestAccEventarcTrigger_BasicHandWritten|TestAccCloudFunctionsFunction_vpcConnector|TestAccRedisInstance_redisInstanceFullExample|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccSqlUser_postgresIAM Please fix these to complete your PR |
TestAccRedisInstance_redisInstanceMrrExample is failing with the errors:
Probably the test needs to be updated to have a valid value. Additionally, TestAccRedisInstance_redisInstanceFullExample is failing with the error:
UPDATE: since the default depends on whether the user has a basic account or not, this will need to use default_from_api. |
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.
see above comment - forgot to leave it as a change request
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 2 files changed, 88 insertions(+), 2 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccEventarcTrigger_BasicHandWritten|TestAccRedisInstance_redisInstanceMrrExample|TestAccRedisInstance_redisInstanceFullExample|TestAccRedisInstance_update|TestAccRedisInstance_redisInstanceAuthEnabled|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=221572 |
Tests failed during RECORDING mode: TestAccEventarcTrigger_BasicHandWritten|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccSqlUser_postgresIAM Please fix these to complete your PR |
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.
lgtm - remaining failures are unrelated
* Redis MRR Support * [Bug fix]Add locationId back to instance properties * Update api.yaml * Update redis_instance_mrr.tf.erb * Update api.yaml * Update terraform.yaml * Update api.yaml Co-authored-by: Himani Khanduja <khimani@google.com>
Changes to support Multi read replica feature on Cloud Memorystor redis
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)