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 subnetwork secondary ip ranges beta feature #310

Merged
merged 11 commits into from
Aug 9, 2017

Conversation

rosbo
Copy link
Contributor

@rosbo rosbo commented Aug 8, 2017

Fixes #287

peer-programmed w/ @rileykarson on this PR.

cc/ @danawillow

@rosbo rosbo requested a review from rileykarson August 8, 2017 18:13
@@ -68,6 +77,26 @@ func resourceComputeSubnetwork() *schema.Resource {
Optional: true,
},

"secondary_ip_range": &schema.Schema{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if it will ever make a difference, but this should probably be marked ForceNew as well as it's children.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@rileykarson
Copy link
Collaborator

LGTM 👍, not sure if we should get another reviewer or not here

TF_ACC=1 go test ./google -v -run=TestAccComputeSubnetwork -timeout 120m
=== RUN   TestAccComputeSubnetwork_importBasic
--- PASS: TestAccComputeSubnetwork_importBasic (63.93s)
=== RUN   TestAccComputeSubnetwork_basic
--- PASS: TestAccComputeSubnetwork_basic (64.90s)
=== RUN   TestAccComputeSubnetwork_update
--- PASS: TestAccComputeSubnetwork_update (61.32s)
=== RUN   TestAccComputeSubnetwork_secondaryIpRanges
--- PASS: TestAccComputeSubnetwork_secondaryIpRanges (74.19s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	264.553s

I'll follow up with another PR cleaning up Beta operations after this.

@rosbo rosbo requested a review from danawillow August 8, 2017 19:33
@rosbo
Copy link
Contributor Author

rosbo commented Aug 8, 2017

I added @danawillow to have another pair of eyes looking at this PR.

@@ -256,3 +333,41 @@ func resourceComputeSubnetworkImportState(d *schema.ResourceData, meta interface

return []*schema.ResourceData{d}, nil
}

func createBetaSubnetID(s *computeBeta.Subnetwork) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This and createSubnetID should probably live in the same file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved createSubnetID to resource_compute_subnetwork.go

@@ -28,6 +28,17 @@ func compareSelfLinkRelativePaths(k, old, new string, d *schema.ResourceData) bo
return false
}

// This method should only be used if the self link references a global resource.
func compareSelfLinkResourceNames(k, old, new string, d *schema.ResourceData) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait I'm confused why compareSelfLinkRelativePaths doesn't work on global resources.

Network self links look like https://www.googleapis.com/compute/v1/projects/project/global/networks/network, so comparing the relative one would be comparing project/global/networks/network which should work just fine, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The network field can be a self_link or the name of the network.
This method works when the field can be either name or self_link.

If we were using compareSelfLinkRelativePaths, then we would get a diff if we specify the network using its name in the config.

mynetwork != project/global/networks/mynetwork

I updated the comment to make it clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the function also be renamed since it's not necessarily comparing self links?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about compareGlobalSelfLinkOrResourceName?

Copy link
Contributor

Choose a reason for hiding this comment

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

sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed.

@rosbo rosbo merged commit 52daf40 into hashicorp:master Aug 9, 2017
@rosbo rosbo deleted the subnetwork-ip-aliasing-beta branch August 9, 2017 22:02
z1nkum pushed a commit to z1nkum/terraform-provider-google that referenced this pull request Aug 15, 2017
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for google_compute_subnetwork IP Aliasing Beta feature
3 participants