-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Promote region* compute services to GA #3381
Promote region* compute services to GA #3381
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 23 files changed, 11363 insertions(+), 33 deletions(-)) |
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. |
Picking a reviewer randomly because of https://github.com/GoogleCloudPlatform/magic-modules/issues/3055. |
Hi @ctavan, thanks for your submission! |
8a93245
to
8eed3c8
Compare
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. @c2thorn, please review this PR or find an appropriate assignee. |
I did the rebase and the necessary changes due to #3379. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 24 files changed, 12084 insertions(+), 33 deletions(-)) |
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. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 24 files changed, 12084 insertions(+), 33 deletions(-)) |
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.
Caught a couple of failing examples/tests here that I believe you can just revert, as they rely on beta-only fields. If it is your intention to promote these fields to GA as well, I recommend to separate them out into different PR's so they are easier to track/discuss.
templates/terraform/examples/region_health_check_http_logs.tf.erb
Outdated
Show resolved
Hide resolved
third_party/terraform/tests/resource_compute_region_backend_service_test.go.erb
Show resolved
Hide resolved
8eed3c8
to
f94349f
Compare
Thanks for the review and sorry for not testing this properly before submitting. I think I got this sorted out now (see comments above) but did not extract the changes into a separate PR since the adjustments looked really straightforward. Would you still like me to do that? If so, how should I split the changes up? |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 15923 insertions(+), 223 deletions(-)) |
@ctavan I appreciate your cooperation and patience on this. There are quite a few changes here, but manual review looks good. Do you mind rebasing one more time, and then I'll be able to run tests against both providers. Once that clears, I can give the LGTM and merge. |
…nd_service Currently there only seem to be two features in backend services that are not GA: --- v1.json +++ beta.json @@ -62,6 +62,10 @@ "enable": boolean, "sampleRate": number }, + "securitySettings": { + "authentication": string, + "subjectAltNames": [string] + }, "localityLbPolicy": enum, "consistentHash": { "httpCookie": { @@ -76,6 +80,10 @@ "minimumRingSize": string }, "circuitBreakers": { + "connectTimeout": { + "seconds": string, + "nanos": integer + }, "maxRequestsPerConnection": integer, "maxConnections": integer, "maxPendingRequests": integer,
f94349f
to
2d70c92
Compare
Done @c2thorn |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 27 files changed, 15921 insertions(+), 223 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 28 files changed, 16125 insertions(+), 223 deletions(-)) |
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.
A couple of more examples needed to be set to GA, but it looks good now
Thank you for your efforts in completing my incomplete work @c2thorn 😇! |
@ctavan No worries, thanks for contributing! |
Many of the
region*
resources are still marked as beta (at least for terraform). Some beta-restrictions were lifted in the past:But others still seem to be missing. Anything holding us back from promoting all the
Region*
resources to GA?Release Note Template for Downstream PRs (will be copied)