provider/google: Changed network argument in google_compute_instance_group as optional #13493

tmshn · 2017-04-09T14:41:07Z

If you want to add an instance group to a backend service, the network filed of the instance group must be set.
Formerly this field is set based on the network where instances are in (this is computed by Google API). It means; if you create the empty instance group, the network field will be empty, and you cannot add it to a backend service.

In this PR, I changed network field in google_compute_instance_group from the computed attribute to the configurable optional argument.
Now user can specify network the group is in and add it to a backend service.

…group as optional

danawillow

Thanks for the PR, @tmshn! I have a few small pieces of feedback, but this is nice and simple and should be able to be merged pretty quickly :)

danawillow · 2017-04-20T17:03:05Z

builtin/providers/google/resource_compute_instance_group.go

@@ -128,6 +130,10 @@ func resourceComputeInstanceGroupCreate(d *schema.ResourceData, meta interface{}
 		instanceGroup.NamedPorts = getNamedPorts(v.([]interface{}))
 	}

+	if v, ok := d.GetOk("network"); ok {
+		instanceGroup.Network = v.(string)


In other terraform resources, we have the network parameter be the name of the network and then do a lookup. For consistency, I think we should do that here too. See https://github.com/hashicorp/terraform/blob/master/builtin/providers/google/resource_compute_instance.go#L479 for an example.

danawillow · 2017-04-20T17:04:13Z

builtin/providers/google/resource_compute_instance_group_test.go

@@ -201,7 +211,34 @@ func testAccComputeInstanceGroup_named_ports(n string, np map[string]int64, inst
 	}
 }

-func testAccComputeInstanceGroup_basic(instance string) string {
+func testAccComputeInstanceGroup_network(n string, network string, instanceGroup *compute.InstanceGroup) resource.TestCheckFunc {


nitpick: can you rename this to something like testAccComputeInstanceGroup_networkExists? Check functions (in my opinion) should be more boolean-like in name to make it clear that it's a check and not a config.

danawillow · 2017-04-20T17:06:04Z

builtin/providers/google/resource_compute_instance_group_test.go

+		name = "%s-empty-with-network"
+		network = "%s"
+		zone = "us-central1-c"
+			named_port {


nit: indentation

…roup_test.go

…testAccComputeInstanceGroup_hasCorrectNetwork

tmshn · 2017-05-02T04:59:45Z

@danawillow Hey, sorry for the late reaction, but I fixed the points you reviewed! 👍 So could you check again?

Actually the first point, the one about the network parameter to take name rather than URL, will be backward incompatible; users who expect network argument to be URL should change its usage.
Is it OK? I'm happy to do extra work if needed.

danawillow · 2017-05-04T20:19:03Z

Oh, good point about backwards incompatibility- I think you're right, sorry to lead you down the wrong path.

…uteInstanceGroup_basic

tmshn · 2017-05-05T06:30:13Z

@danawillow No problem! I dropped the last commit (and bit modified the next last one).

danawillow · 2017-05-08T17:36:56Z

builtin/providers/google/resource_compute_instance_group_test.go

+		"GCLOUD_PROJECT",
+		"CLOUDSDK_CORE_PROJECT",
+	}
+	var networkUrl = fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/global/networks/%s", multiEnvSearch(projs), networkName)


This test doesn't feel very basic anymore. Mind creating a new test for this? I also think creating the network from within the test would be nicer than relying on the default network and having to read the project from the environment.

tmshn · 2017-05-20T17:17:11Z

@danawillow Divided the test for the network 👍

danawillow · 2017-06-06T00:21:16Z

builtin/providers/google/resource_compute_instance_group_test.go

@@ -201,6 +227,44 @@ func testAccComputeInstanceGroup_named_ports(n string, np map[string]int64, inst
 	}
 }

+func testAccComputeInstanceGroup_hasCorrectNetwork(n_ig string, n_nw string, instanceGroup *compute.InstanceGroup) resource.TestCheckFunc {


style nit: I'm not a huge fan of mixing types of cases- let's leave everything camelCase

Okay, but what does "everything" mean? "Every function names" including the ones I didn't modify? Or just "every letter" of this function name?

oh I just meant the variables (n_ig, etc.)

Oh sorry, yeah, I got it 😅

Fixed 💪 28f0d5e

danawillow · 2017-06-06T00:21:33Z

builtin/providers/google/resource_compute_instance_group_test.go

+
+	resource "google_compute_instance_group" "with_instance" {
+		description = "Terraform test instance group"
+		name = "%[1]s-with-insntance"


typo: instance

Oops, sorry! Fixed -> 4aa065c

…stance_group_test.go

danawillow · 2017-06-07T18:19:29Z

TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeInstanceGroup -timeout 120m
=== RUN   TestAccComputeInstanceGroup_basic
--- PASS: TestAccComputeInstanceGroup_basic (136.93s)
=== RUN   TestAccComputeInstanceGroup_update
--- PASS: TestAccComputeInstanceGroup_update (165.93s)
=== RUN   TestAccComputeInstanceGroup_outOfOrderInstances
--- PASS: TestAccComputeInstanceGroup_outOfOrderInstances (116.40s)
=== RUN   TestAccComputeInstanceGroup_network
--- PASS: TestAccComputeInstanceGroup_network (198.64s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	618.011s

ghost · 2020-04-11T02:20:28Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

provider/google: Changed network argument in google_compute_instance_…

bc66484

…group as optional

radeksimko added enhancement provider/google-cloud labels Apr 9, 2017

tmshn mentioned this pull request Apr 19, 2017

provider/google: Fixed instance group manager to allow 0 instance #13492

Closed

danawillow suggested changes Apr 20, 2017

View reviewed changes

tmshn added 2 commits May 2, 2017 13:50

provider/google: Fixed the indentation in resource_compute_instance_g…

fe1c0d6

…roup_test.go

provider/google: Just naming: testAccComputeInstanceGroup_network -> …

b8c6c40

…testAccComputeInstanceGroup_hasCorrectNetwork

tmshn force-pushed the google_instancegroup_network branch from 18ba343 to 036535d Compare May 5, 2017 06:10

provider/google: Added network check with basic config in TestAccComp…

086059f

…uteInstanceGroup_basic

tmshn force-pushed the google_instancegroup_network branch from 036535d to 086059f Compare May 5, 2017 06:26

danawillow reviewed May 8, 2017

View reviewed changes

provider/google: split acc test for instance group

c65eca2

danawillow reviewed Jun 6, 2017

View reviewed changes

tmshn added 2 commits June 6, 2017 13:58

provider/google: Fixed typo in resource_compute_instance_group_test.go

4aa065c

provider/google: Modify var names to camelCase in resource_compute_in…

28f0d5e

…stance_group_test.go

danawillow approved these changes Jun 7, 2017

View reviewed changes

danawillow merged commit 0b6518a into hashicorp:master Jun 7, 2017

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/google: Changed network argument in google_compute_instance_group as optional #13493

provider/google: Changed network argument in google_compute_instance_group as optional #13493

tmshn commented Apr 9, 2017

danawillow left a comment

danawillow Apr 20, 2017

danawillow Apr 20, 2017

danawillow Apr 20, 2017

tmshn commented May 2, 2017

danawillow commented May 4, 2017

tmshn commented May 5, 2017

danawillow May 8, 2017

tmshn commented May 20, 2017

danawillow Jun 6, 2017

tmshn Jun 6, 2017

danawillow Jun 6, 2017

tmshn Jun 7, 2017

tmshn Jun 7, 2017

danawillow Jun 6, 2017

tmshn Jun 6, 2017

danawillow commented Jun 7, 2017

ghost commented Apr 11, 2020

provider/google: Changed network argument in google_compute_instance_group as optional #13493

provider/google: Changed network argument in google_compute_instance_group as optional #13493

Conversation

tmshn commented Apr 9, 2017

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshn commented May 2, 2017

danawillow commented May 4, 2017

tmshn commented May 5, 2017

Choose a reason for hiding this comment

tmshn commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danawillow commented Jun 7, 2017

ghost commented Apr 11, 2020