-
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
Add WEIGHTED_MAGLEV to localityLbPolicy enum, and update associated docstrings. #7444
Conversation
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.
Changes look good to me (and sorry for the delay).
The one thing I wanted to check in on before approving: can you help me understand which parts of this are beta-only? I was expecting either:
- All additions being GA
- The new enum being marked in the description as "beta only" in some way
Thanks for the review! All additions here are supposed to be GA. Was the point of confusion due to the |
Ah ok, yep, the |
Oh also, now that I'm seeing the conflict, we made a project-wide change that will require you to move the config you have in |
9027e49
to
bcc5a9d
Compare
Thanks for the explanation. In hindsight, it makes sense to me that Think I made all the requested changes, and included the updated changes from main. PTAL. |
/gcbrun |
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. Terraform GA: Diff ( 5 files changed, 150 insertions(+), 14 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccLoggingBucketConfigProject_analyticsEnabled|TestAccLoggingBucketConfig_CreateBuckets_withCustomId|TestAccLoggingBucketConfigProject_cmekSettings|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeRegionBackendService_regionBackendServiceExternalWeightedExample|TestAccDataSourceDnsManagedZone_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: 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.
Code looks good, just the one test failure that needs to be fixed
/gcbrun |
Just fixed the test. Sorry about that, I was negligent and didn't re-run tests after the merge yesterday. |
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. Terraform GA: Diff ( 5 files changed, 150 insertions(+), 14 deletions(-)) |
Tests analyticsTotal tests: 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 testsTestAccComputeRegionBackendService_regionBackendServiceExternalWeightedExample|TestAccLoggingBucketConfigProject_analyticsEnabled|TestAccDataSourceDnsManagedZone_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: 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.
Oh no worries, thanks for fixing. LGTM
…ocstrings. (GoogleCloudPlatform#7444) * Add WEIGHTED_MAGLEV to localityLbPolicy enum, and update associated docstrings. * Example for WEIGHTED_MAGLEV uses HTTP health check.
…ocstrings. (GoogleCloudPlatform#7444) * Add WEIGHTED_MAGLEV to localityLbPolicy enum, and update associated docstrings. * Example for WEIGHTED_MAGLEV uses HTTP health check.
This PR adds WEIGHTED_MAGLEV to the enum for localist_lb_policy, resolving b/272540261 and unblocking NetLB Weighted Load Balancing GA.
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.