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

Adding next_hop_ilb attribute to compute route resource #2215

Merged
merged 8 commits into from
Aug 26, 2019
Merged

Adding next_hop_ilb attribute to compute route resource #2215

merged 8 commits into from
Aug 26, 2019

Conversation

Madankapoor
Copy link
Contributor

This adds the beta feature to the route resource for next-hop internal load balancer. https://cloud.google.com/compute/docs/reference/rest/beta/routes/insert

Release Note for Downstream PRs (will be copied)

compute: The compute routes includes next_hop_ilb attribute support in beta.

@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. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@rambleraptor, please review this PR or find an appropriate assignee.

@rambleraptor
Copy link
Contributor

Hello! I'm going to kick off our CI and put @rileykarson on this (since it's a Terraform-only contribution)

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of 7072493.

Pull request statuses

No diff detected in terraform-google-conversion.
No diff detected in terraform-provider-google.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1076

@rileykarson
Copy link
Member

Hey @Madankapoor!

Do you mind adding an example using this field? If you add an example definition based on the Example type, MM will automatically generate a test based on the example's config.

@Madankapoor
Copy link
Contributor Author

Hi @rileykarson, Thank you for your feedback. I have added a test case as required.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 52e684d.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google#4311

@rileykarson
Copy link
Member

Thanks @Madankapoor! I'm passing review off to @slevenick because they have more background on the feature than me.

@rileykarson rileykarson requested review from slevenick and removed request for rileykarson August 21, 2019 17:28
Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Looks good, thanks! Just needs a simple change and then I'll take care of merging in the downstreams.

products/compute/terraform.yaml Show resolved Hide resolved
subnetwork = "${google_compute_subnetwork.default.name}"
}

resource "google_compute_route" "default" {
Copy link
Contributor

Choose a reason for hiding this comment

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

primary_resource_id is supposed to be used within the example template to allow terraform to access the resource's id for import test purposes. It doesn't get used in beta-only examples, but can you change it so that when this moves to the google provider it will work?

Example: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/templates/terraform/examples/node_template_server_binding.tf.erb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya i have added primary id

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace "google_compute_route" "default" with "google_compute_route" "<%= ctx[:primary_resource_id] %>" here

It allows us to access the name of the terraform resource for import later. This is different than the name field on the resource itself, as we need to know the name used to identify the block for testing import

@Madankapoor
Copy link
Contributor Author

@slevenick I have made requested changes and responded back to the questions. Does it seem good ?

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 18bec7d.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: modular-magician/inspec-gcp#190

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Just a couple small things, but once they are fixed I will merge!

@@ -0,0 +1,52 @@
resource "google_compute_network" "default" {
provider = "google-beta"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you fix the spacing on this block? Seems to have gotten mixed up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya fixed it.

subnetwork = "${google_compute_subnetwork.default.name}"
}

resource "google_compute_route" "default" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you replace "google_compute_route" "default" with "google_compute_route" "<%= ctx[:primary_resource_id] %>" here

It allows us to access the name of the terraform resource for import later. This is different than the name field on the resource itself, as we need to know the name used to identify the block for testing import

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 000eeea.

Pull request statuses

terraform-provider-google-beta already has an open PR.
No diff detected in terraform-google-conversion.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Thanks for the addition!

@modular-magician modular-magician merged commit d481725 into GoogleCloudPlatform:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants