Skip to content
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

Enable EXTERNAL_MANAGED resource examples for GA #5660

Merged

Conversation

AlexanderEllis
Copy link
Contributor

@AlexanderEllis AlexanderEllis commented Jan 27, 2022

One last change for the Global External HTTP(S) Load Balancer to remove the beta-only requirement for the examples and tests. This is for the EXTERNAL_MANAGED load_balancing_scheme option for google_compute_global_forwarding_rule and google_compute_backend_service.

Really fixes hashicorp/terraform-provider-google#10858

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

compute: promoted `EXTERNAL_MANAGED` value for `load_balancing_scheme` in `google_compute_backend_service ` and `google_compute_global_forwarding_rule` to GA

@modular-magician
Copy link
Collaborator

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.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 112 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 4 files changed, 2 insertions(+), 14 deletions(-))
TF Validator: Diff ( 4 files changed, 112 insertions(+), 6 deletions(-))
TF OiCS: Diff ( 2 files changed, 6 deletions(-))

@shuyama1 shuyama1 requested review from shuyama1 and removed request for melinath January 27, 2022 19:03
@shuyama1
Copy link
Member

/gcbrun

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 112 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 4 files changed, 2 insertions(+), 14 deletions(-))
TF Validator: Diff ( 4 files changed, 112 insertions(+), 6 deletions(-))
TF OiCS: Diff ( 2 files changed, 6 deletions(-))

@shuyama1
Copy link
Member

/gcbrun

@shuyama1
Copy link
Member

Hi @AlexanderEllis! Looks like the feature is already implemented in GA, but only the tests/examples are in beta, right? Sorry that I did not notice we were only testing in beta in the previous PR!

@AlexanderEllis
Copy link
Contributor Author

Yup that's right! The feature has actually only been enabled in the GA API since my last PR, so it made sense to only test via the beta API at the time. They're now moving towards GA (starting with availability in the API), which is why I'm following up with this 👍

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 112 insertions(+), 6 deletions(-))
Terraform Beta: Diff ( 4 files changed, 2 insertions(+), 14 deletions(-))
TF Validator: Diff ( 4 files changed, 112 insertions(+), 6 deletions(-))
TF OiCS: Diff ( 2 files changed, 6 deletions(-))

@shuyama1
Copy link
Member

Yup that's right! The feature has actually only been enabled in the GA API since my last PR, so it made sense to only test via the beta API at the time. They're now moving towards GA (starting with availability in the API), which is why I'm following up with this 👍

Okay! I just kick off the tests. Looks like the last one got timed out! Sorry about it.

@shuyama1
Copy link
Member

Yeah! I remember about it! I tested it locally with the compute backend service in the GA provider in the first PR and it failed. It makes sense now. Thanks @AlexanderEllis!

Copy link
Member

@shuyama1 shuyama1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have beta only in the description of load_balancing_scheme in google_compute_global_forwarding_rule (https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/products/compute/api.yaml#L4267)
Should we also modify this?
I am trying to understand here. So the field was not just beta only but EXTERNAL_MANAGED was, and now we want to GA this option, right?

@AlexanderEllis
Copy link
Contributor Author

Yup that's right, only EXTERNAL_MANAGED was beta, and now we want to GA it (which is really GA'ing the examples and tests I believe).

And as far as that "Beta only", that's a good question. AFAICT it looks like that might have been added just in reference to the following PRIVATE_SERVICE_CONNECT configuration? 5f8d650 in #4358:

image

@shuyama1
Copy link
Member

shuyama1 commented Feb 1, 2022

Yup that's right, only EXTERNAL_MANAGED was beta, and now we want to GA it (which is really GA'ing the examples and tests I believe).

And as far as that "Beta only", that's a good question. AFAICT it looks like that might have been added just in reference to the following PRIVATE_SERVICE_CONNECT configuration? 5f8d650 in #4358:

image

I think you are absolutely right! Then everything looks good to me! Thanks @AlexanderEllis

@shuyama1 shuyama1 self-requested a review February 1, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform support for Global External HTTP(S) Load Balancer
3 participants