provider/google: datasource subnetwork and network #12442

mathieuherbert · 2017-03-04T10:40:21Z

Hi,

This PR adds google_compute_subnetwork and google_compute_network.

Thanks for your review :)

TF_ACC=true go test -v github.com/hashicorp/terraform/builtin/providers/google -run ^TestAccDataSourceGoogleNetwork$
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	0.016s

TF_ACC=true go test -v github.com/hashicorp/terraform/builtin/providers/google -run TestAccDataSourceGoogleSubnetwork
=== RUN   TestAccDataSourceGoogleSubnetwork
--- PASS: TestAccDataSourceGoogleSubnetwork (86.76s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	86.777s

danawillow

Thanks @mathieuherbert! This looks really good so far; most of my change requests are minor. I didn't get a chance to read all the way through the end of the documentation so I'll be sure to get to that next week.

danawillow · 2017-03-31T01:03:05Z

builtin/providers/google/data_source_google_compute_network.go

+				Type:     schema.TypeString,
+				Optional: true,
+			},
+			"subnetworks_self_links": {


nit: mind adding a newline right before here for consistency?

danawillow · 2017-03-31T01:07:22Z

builtin/providers/google/data_source_google_compute_network_test.go

+			return fmt.Errorf("root module has no resource called %s", name)
+		}
+
+		networkOrigin, ok := s.RootModule().Resources["google_compute_network.foobar"]


A few ideas to make this function more reusable (in case more tests come for this down the road):

google_compute_network.foobar could be a parameter

All attributes could be compared resource vs data source (right now id and self_link are doing that, but name and description are using hardcoded values)
Sound reasonable?

danawillow · 2017-03-31T01:10:59Z

builtin/providers/google/data_source_google_compute_subnetwork.go

+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"network_self_link": &schema.Schema{


Hmm, so there's an equivalent field in the subnetwork resource which is just called network. I like that your name is more descriptive, but for consistency's sake it may be better to just go with that.

danawillow · 2017-03-31T01:12:09Z

builtin/providers/google/data_source_google_compute_network_test.go

+	})
+}
+
+func testAccDataSourceGoogleNetworktCheck(name string) resource.TestCheckFunc {


Does the "t" after Network stand for anything or is that a typo?

danawillow · 2017-03-31T01:12:27Z

builtin/providers/google/data_source_google_compute_subnetwork_test.go

+	})
+}
+
+func testAccDataSourceGoogleSubnetworktCheck(name string) resource.TestCheckFunc {


Same comment about the "t"

danawillow · 2017-03-31T01:13:24Z

builtin/providers/google/data_source_google_compute_subnetwork_test.go

+		if !ok {
+			return fmt.Errorf("root module has no resource called %s", name)
+		}
+		network, ok := s.RootModule().Resources["google_compute_network.foobar"]


Same comments here as the other test

danawillow · 2017-03-31T01:14:09Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+---
+layout: "google"
+page_title: "Google: google_compute_network"
+sidebar_current: "docs-google-datasource-compute-etwork"


typo: network

danawillow · 2017-03-31T01:15:10Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+
+# google\_compute\_network
+
+Get a network within GCE from his name.


danawillow · 2017-03-31T01:15:59Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+
+## Example Usage
+
+```js


You can use tf for the language now! (I think this is a relatively new development)

danawillow · 2017-03-31T01:17:29Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+
+## Attributes Reference
+
+In addition to the arguments listed above, the following computed attributes are


The other gcp data sources docs seem to be using this phrasing: "The following attributes are exported:"

mathieuherbert · 2017-04-05T20:34:52Z

Hi @danawillow,
Thank you for the review, in the last 2 commits I make changes thanks to your comments.

danawillow

Sorry for all the comments! Just trying to make sure the code that's checked in is the highest possible quality :)

danawillow · 2017-04-06T17:00:51Z

builtin/providers/google/data_source_google_compute_network_test.go

 				),
 			},
 		},
 	})
 }

-func testAccDataSourceGoogleNetworktCheck(name string) resource.TestCheckFunc {
+func testAccDataSourceGoogleNetworkCheck(name string, network_name string) resource.TestCheckFunc {


I'd love it if this test (and the subnetwork one) had clearer names for things. For example, "name" could be the name of anything, so how about something like data_source_name. And then network_name could be resource_name.

From there, attr could be renamed to ds_attr and then you could have a similar variable called resource_attr. Then it would be super clear that at the end, you're comparing what was set in the data source to what was set in the resource. Does that make sense?

danawillow · 2017-04-06T17:01:24Z

builtin/providers/google/data_source_google_compute_network_test.go

 	return func(s *terraform.State) error {
 		rs, ok := s.RootModule().Resources[name]
 		if !ok {
 			return fmt.Errorf("root module has no resource called %s", name)
 		}

-		networkOrigin, ok := s.RootModule().Resources["google_compute_network.foobar"]
+		networkOrigin, ok := s.RootModule().Resources[network_name]
 		if !ok {
 			return fmt.Errorf("can't find google_compute_network.foobar in state")


replace google_compute_network.foobar with the variable name containing it

danawillow · 2017-04-06T17:02:30Z

builtin/providers/google/data_source_google_compute_network.go

+				Type:     schema.TypeString,
+				Computed: true,
+			},
+			"project": &schema.Schema{


formatting nitpick: can you add a newline before "project"?

danawillow · 2017-04-06T17:24:25Z

builtin/providers/google/data_source_google_compute_network_test.go

+
+		attr := rs.Primary.Attributes
+
+		if attr["id"] != networkOrigin.Primary.Attributes["id"] {


Since all the checks are approximately the same, you could add a function that just does one check and returns the error. So it would take in the attr name, the data source attr, and the resource attr and return nil or an error. https://github.com/hashicorp/terraform/blob/master/builtin/providers/google/resource_container_cluster_test.go#L181 has an example of this, though it's a fair bit more complicated than what you'll want here. Let me know if you want any help constructing this.

danawillow · 2017-04-06T17:28:43Z

builtin/providers/google/data_source_google_compute_subnetwork.go

+	d.Set("gateway_address", subnetwork.GatewayAddress)
+	d.Set("network", subnetwork.Network)
+
+	//To put subnet id see resource compute subnetnetwork details


I'm not sure what this comment means. Are you trying to say that the creation of the subnet id occurs in resource_compute_subnetwork? If so, maybe phrasing like subnet id creation is defined in resource_compute_subnetwork.go would be more clear.

danawillow · 2017-04-06T18:18:36Z

builtin/providers/google/data_source_google_compute_network.go

+		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 {
+			// The resource doesn't exist anymore
+
+			return fmt.Errorf("Network Not Found")


Maybe put the network name in the error message

danawillow · 2017-04-06T18:19:50Z

builtin/providers/google/data_source_google_compute_subnetwork_test.go

+		if !ok {
+			return fmt.Errorf("root module has no resource called %s", name)
+		}
+		network, ok := s.RootModule().Resources[subnetwork_name]


Shouldn't this be network_name?

danawillow · 2017-04-06T18:21:47Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+
+* `description` - Description of this network.
+
+* `ip_v4_range` - (DEPRECATED) The IPv4 address range that machines in this network


If this is deprecated do you actually need to be exporting it?

(also the field name you used in the data source was ipv4_range)

danawillow · 2017-04-06T18:22:09Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+* `description` - Description of this network.
+
+* `ip_v4_range` - (DEPRECATED) The IPv4 address range that machines in this network
+   are assigned to, represented as a CIDR block..


extra period

danawillow · 2017-04-06T18:22:23Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+
+* `gateway_ipv4` - The IP address of the gateway.
+
+* `subnetworks_self_links` - the list of subnetworks which belongs to the network


grammar nitpick: "which belong"

mathieuherbert · 2017-04-06T20:16:35Z

@danawillow thank you for the review.
Your comments are very interesting for me !
About tests, I create an array with every fields that I want to tests and iterate over it.
Also I remove ip_v4_range field.

danawillow

Almost there! Also it looks like some merge conflicts snuck in recently.

danawillow · 2017-04-12T23:36:52Z

website/source/docs/providers/google/d/datasource_compute_network.html.markdown

+## Example Usage
+
+```tf
+datasource "google_compute_network" "my-network" {


datasource -> data

danawillow · 2017-04-12T23:37:05Z

website/source/docs/providers/google/d/datasource_compute_subnetwork.html.markdown

+## Example Usage
+
+```tf
+datasource "google_compute_subnetwork" "my-subnetwork" {


mathieuherbert · 2017-04-13T20:06:46Z

@danawillow I did a rebase which fix the conflict
Also I fix the change requested on documentation

danawillow · 2017-04-13T20:24:22Z

Thanks @mathieuherbert! This looks good to me; I'll merge it now.

ghost · 2020-04-14T01:54:45Z

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.

stack72 added enhancement new-data-source provider/google-cloud labels Mar 6, 2017

danawillow mentioned this pull request Mar 30, 2017

[WIP] Added data source google_compute_subnetworks #12954

Closed

2 tasks

danawillow suggested changes Mar 31, 2017

View reviewed changes

danawillow suggested changes Apr 6, 2017

View reviewed changes

danawillow suggested changes Apr 12, 2017

View reviewed changes

mathieuherbert added 8 commits April 13, 2017 21:45

first version of this datasource

c8a413a

add network and subnetwork datasource and documentation

72ea55d

modify sidebar reference in documentation

a1a0938

fix elements after review on network and subnetwork datasources

3bbe97f

fix fmt on Google provider.go

4e166c2

modify code with the review

fae6516

modify documentation layout order

c36c742

fix alphabetic order in provider.go

7d81973

mathieuherbert force-pushed the datasource-network-gce branch from 5eec194 to 7d81973 Compare April 13, 2017 19:49

fix rebase issue and documentation datasource => data

191d090

danawillow approved these changes Apr 13, 2017

View reviewed changes

danawillow merged commit 25cbbde into hashicorp:master Apr 13, 2017

mathieuherbert deleted the datasource-network-gce branch April 13, 2017 20:32

ghost locked and limited conversation to collaborators Apr 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/google: datasource subnetwork and network #12442

provider/google: datasource subnetwork and network #12442

mathieuherbert commented Mar 4, 2017

danawillow left a comment

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

danawillow Mar 31, 2017

mathieuherbert commented Apr 5, 2017

danawillow left a comment

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

danawillow Apr 6, 2017

mathieuherbert commented Apr 6, 2017

danawillow left a comment

danawillow Apr 12, 2017

danawillow Apr 12, 2017

mathieuherbert commented Apr 13, 2017

danawillow commented Apr 13, 2017

ghost commented Apr 14, 2020


		# google\_compute\_network

		Get a network within GCE from his name.


		## Attributes Reference

		In addition to the arguments listed above, the following computed attributes are


		attr := rs.Primary.Attributes

		if attr["id"] != networkOrigin.Primary.Attributes["id"] {


		* `description` - Description of this network.

		* `ip_v4_range` - (DEPRECATED) The IPv4 address range that machines in this network


		* `gateway_ipv4` - The IP address of the gateway.

		* `subnetworks_self_links` - the list of subnetworks which belongs to the network

provider/google: datasource subnetwork and network #12442

provider/google: datasource subnetwork and network #12442

Conversation

mathieuherbert commented Mar 4, 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

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

mathieuherbert commented Apr 5, 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

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

mathieuherbert commented Apr 6, 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

mathieuherbert commented Apr 13, 2017

danawillow commented Apr 13, 2017

ghost commented Apr 14, 2020