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

Add EXTERNAL_MANAGED option to global forwarding rule and add example #5611

Merged
merged 2 commits into from
Jan 26, 2022

Conversation

AlexanderEllis
Copy link
Contributor

@AlexanderEllis AlexanderEllis commented Jan 12, 2022

This adds the EXTERNAL_MANAGED load balancing scheme option to the Global Forwarding Rule resource and adds an example that will generate tests.

This option was added in the DCL (the change was merged in a previous PR #5586), but I'm also adding it here so that it automatically generates the test and examples.

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: Added `EXTERNAL_MANAGED` as option for `load_balancing_scheme` in `google_compute_global_forwarding_rule` resource

@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.

@shuyama1, 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 ( 5 files changed, 25 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 4 files changed, 7 insertions(+), 3 deletions(-))

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.

Hi @AlexanderEllis! Would you mind following the instruction here to update resources coming from DCL. I believe running make upgrade-dcl and then make serialize should make changes generated. Thanks a lot!

@AlexanderEllis
Copy link
Contributor Author

Hi @shuyama1 , sure thing! I thought I had done that before, but I don't think it ended up making any additional changes beyond the 4 files (the go.mod and go.sum files).

I tried running it again, but I think I may have ran into an issue with make serialize:

image

Not sure if this is related to my change, but happy to debug further.

@AlexanderEllis
Copy link
Contributor Author

Ah OK I think I figured it out, I hadn't rebased with the other changes in magic-modules. I did so, and it looks like it still only generated the four files: 2582c98

I believe I added the DCL right, and it looks like the auto import in the DCL repo included the samples: GoogleCloudPlatform/declarative-resource-client-library@f92a0d2 . Am I missing something with the import?

@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 ( 3 files changed, 14 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 3 files changed, 14 insertions(+), 5 deletions(-))

@AlexanderEllis
Copy link
Contributor Author

On further review, I think this was already propagated in a previous PR upgrading the version (https://github.com/hashicorp/terraform-provider-google-beta/pull/3987/files#diff-855513ce3d7fa58a1e5c7435ff810420698b528e20322f1b96dc8fd049b90b08), so I think this is already set

@shuyama1
Copy link
Member

shuyama1 commented Jan 20, 2022

@AlexanderEllis Sorry for missing this. Somehow, it did not show up on my review list after you made the change. Really apologize for this. You are right. it looks like part of the code is already generated and pushed in a previous PR.

One problem is that this a TF brownfield resource that we don't generate tests and documentation using tpgtools (DCL-based generator). So we have to manually add the tests and update the documentation in the handwritten files (test, docs). Would you mind making those changes against those files. Sorry about the extra work! Please let me know if you have any questions.

Update: Looks like the tests and docs for this resource are generated from mmv1, and therefore the changes should be made from corresponding yaml files but not handwritten files. Updated my comment in case anyone reading this got confused.

@shuyama1
Copy link
Member

Also please re-request my review after you make the changes. Thanks a lot!

@AlexanderEllis AlexanderEllis changed the title Upgrade DCL version to include forwarding-rule change Add EXTERNAL_MANAGED option to global forwarding rule and add example Jan 21, 2022
@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 ( 1 file changed, 58 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 133 insertions(+), 2 deletions(-))
TF OiCS: Diff ( 4 files changed, 146 insertions(+))

@AlexanderEllis
Copy link
Contributor Author

Thanks again for your help, @shuyama1! I think I've updated it correctly here:

  • Added the option in the api.yaml file
  • Added an example with the new EXTERNAL_MANAGED option
  • Added the example in the terraform.yaml file

I also updated the PR and description to better reflect these changes 👍

@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 ( 1 file changed, 58 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 133 insertions(+), 2 deletions(-))
TF OiCS: Diff ( 4 files changed, 146 insertions(+))

@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 ( 1 file changed, 58 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 2 files changed, 133 insertions(+), 2 deletions(-))
TF OiCS: Diff ( 4 files changed, 146 insertions(+))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudbuildWorkerPool_basic|TestAccComputeGlobalForwardingRule_globalForwardingRuleExternalManagedExample|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccServiceNetworkingPeeredDNSDomain_basic You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=247005

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.

@AlexanderEllis The tests passed! LGTM! I've changed a bit in the description and CHANGELOG. Can you confirm those changes are valid? Thank you so much!

@AlexanderEllis
Copy link
Contributor Author

Changes LGTM, thanks again!

@shuyama1
Copy link
Member

Great! Merging now! Thanks for the contribution :)

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