New datasource: google_container_cluster #740

sl1pm4t · 2017-11-15T02:39:08Z

Adds the google_kubernetes_cluster datasource.

This is useful for retrieving the cluster credentials and feeding into the Terraform Kubernetes provider, but has other use cases (e.g. feed cluster CIDR range into firewall/VPC route resources).

I've named the datasource google_kubernetes_cluster rather than google_container_cluster to match the new Google product name, but this means the datasource naming is not consistent with the equivalent resource. Let me know if this is acceptable.

Fixes #732

sl1pm4t · 2017-11-20T19:08:00Z

Hi @danawillow - any chance we could get this reviewed before it gets stale? Thanks!

danawillow · 2017-11-20T19:18:57Z

Hey @sl1pm4t, thanks for the PR! I've been drowning in code reviews lately but I'll try to get to this before the Thanksgiving break (and of course don't let that stop you from continuing to contribute!)

Right now I think keeping the data source name consistent with the resource name is probably the way to go though.

sl1pm4t · 2017-11-20T19:23:45Z

No worries @danawillow, I felt bad asking because I know you're swamped. I really appreciate your massive effort!

sl1pm4t · 2017-11-21T18:37:50Z

Hi @danawillow - I've renamed the datasource and fixed a couple other issues I spotted in the tests, and missing reference to the docs page in google.erb.
Thanks!

danawillow · 2017-11-21T21:05:31Z

google/data_source_google_container_cluster.go

+
+			"cluster_ipv4_cidr": {
+				Type:     schema.TypeString,
+				Optional: true,


Except for name, zone, and project, we probably shouldn't allow setting fields since they don't get used to make the request

danawillow · 2017-11-21T21:06:23Z

google/data_source_google_container_cluster.go

+			"master_auth": {
+				Type:     schema.TypeList,
+				Optional: true,
+				ForceNew: true,


Likewise, I don't think ForceNew really has meaning for data sources

Good point. Will fix this.

danawillow · 2017-11-21T21:09:51Z

google/data_source_google_container_cluster.go

+	}
+}
+
+func datasourceContainerClusterRead(d *schema.ResourceData, meta interface{}) error {


I'm curious why you decided to have only a subset of the resource schema fields in the data source schema. If you had the same set of fields, then you would be able to reuse the read function (https://github.com/terraform-providers/terraform-provider-google/blob/master/google/data_source_google_compute_instance_group.go is a good example of this). If there is a reason why certain schema elements are excluded, I'd love to see a comment explaining why.

Great idea, I hadn't considered using the resources Read func, but that makes a lot of sense.
I stripped down the schema to a subset that I thought would be useful in a datasource, based on the notion if it wasn't going to be useful, remove it so there's less to maintain in the read func. Now that you've pointed out we can re-use the read func from the resource it makes sense to keep the whole schema.

sl1pm4t · 2017-12-11T02:52:38Z

Hi @danawillow - sorry for the delay fixing this up.

As per your suggestion, the datasource now uses the existing google_container_cluster resource read func rather than being re-implemented for the datasource.
It also implements the full resource schema rather than just a subset. This is done by generating the datasource schema directly from the resource schema (see datasourceSchemaFromResourceSchema in google/utils.go) so future maintenance should be minimal if the attributes are added / modified / deleted on the resource.

Thanks!

Add documentation for google_kubernetes_cluster datasource Rename datasource to google_container_cluster To be consistent with the equivalent resource. Rename datasource in docs. google_kubernetes_cluster -> google_container_cluster. Also add reference in google.erb file. WIP Datasource read needs to set an ID, then call resource read func Add additional cluster attributes to datasource schema

Datasource documentation also updated.

sl1pm4t · 2017-12-19T20:56:17Z

I rebased on master branch to resolve conflicts.

danawillow · 2017-12-20T18:49:58Z

google/utils_test.go

+	}
+}
+
+func dummyCustomSetFunc(v interface{}) int {


Looks like this is unused

danawillow · 2017-12-20T18:50:14Z

google/utils.go

+			fallthrough
+		case schema.TypeList:
+			// List & Set types are generally used for 2 cases:
+			// - a list/set of simple primative values (e.g. list of strings)


nit: s/primative/primitive

danawillow · 2017-12-20T18:50:23Z

google/utils.go

+					Schema: datasourceSchemaFromResourceSchema(elem.Schema),
+				}
+			} else {
+				// handle simple primative case


and here :)

danawillow · 2017-12-20T18:58:25Z

google/utils.go

+// example) and therefore the attribute flags were not set appropriately when
+// first added to the schema definition. Currently only supports top-level
+// schema elements.
+func fixDatasourceSchemaFlags(schema map[string]*schema.Schema, required bool, keys ...string) {


Optional idea: it might be a bit more readable in the resources themselves to have two additional fns: addRequiredFieldsToSchema and addOptionalFieldsToSchema (or something along those lines), that just call into this one with the appropriate bool value.

danawillow · 2017-12-20T18:59:14Z

website/docs/d/datasource_container_cluster.html.markdown

+
+# google\_container\_cluster
+
+Get info about a cluster within GCE from its name and zone.


danawillow · 2017-12-20T19:07:27Z

google/utils.go

+// - all attributes have Computed = true
+// - all attributes have ForceNew, Required = false
+// - Validation funcs and attributes (e.g. MaxItems) are not copied
+func datasourceSchemaFromResourceSchema(rs map[string]*schema.Schema) map[string]*schema.Schema {


Great idea, thanks so much for adding this! For a bit more visibility within the provider, it might make more sense to move these helper fns into their own file (datasource_helpers.go or something like that)

sl1pm4t · 2017-12-20T19:57:49Z

@danawillow - thanks for the review. The requested changes have been implemented. 👨‍💻

danawillow

Looks great, thanks so much @sl1pm4t!

* Add google_kubernetes_cluster datasource Add documentation for google_kubernetes_cluster datasource Rename datasource to google_container_cluster To be consistent with the equivalent resource. Rename datasource in docs. google_kubernetes_cluster -> google_container_cluster. Also add reference in google.erb file. WIP Datasource read needs to set an ID, then call resource read func Add additional cluster attributes to datasource schema * Generate datasource schema from resource Datasource documentation also updated. * add test for datasourceSchemaFromResourceSchema * Code review changes

…rp#740) Signed-off-by: Modular Magician <[email protected]>

ghost · 2020-03-30T14:00:39Z

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!

danawillow self-requested a review November 20, 2017 19:19

danawillow self-assigned this Nov 20, 2017

sl1pm4t changed the title ~~New datasource: google_kubernetes_cluster~~ New datasource: google_container_cluster Nov 21, 2017

danawillow suggested changes Nov 21, 2017

View reviewed changes

sl1pm4t added 3 commits December 20, 2017 09:54

Generate datasource schema from resource

770522b

Datasource documentation also updated.

add test for datasourceSchemaFromResourceSchema

110d947

danawillow reviewed Dec 20, 2017

View reviewed changes

Code review changes

7bc8eb1

danawillow approved these changes Dec 20, 2017

View reviewed changes

danawillow merged commit 23ef50f into hashicorp:master Dec 20, 2017

sl1pm4t deleted the gke-cluster-datasource branch December 21, 2017 02:56

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

[terraform] Fix import example for google_project_iam_member (hashico…

8cda854

…rp#740) Signed-off-by: Modular Magician <[email protected]>

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New datasource: google_container_cluster #740

New datasource: google_container_cluster #740

sl1pm4t commented Nov 15, 2017

sl1pm4t commented Nov 20, 2017

danawillow commented Nov 20, 2017

sl1pm4t commented Nov 20, 2017

sl1pm4t commented Nov 21, 2017

danawillow Nov 21, 2017

danawillow Nov 21, 2017

sl1pm4t Nov 21, 2017

danawillow Nov 21, 2017

sl1pm4t Nov 22, 2017 •

edited

Loading

sl1pm4t commented Dec 11, 2017

sl1pm4t commented Dec 19, 2017

danawillow Dec 20, 2017

danawillow Dec 20, 2017

danawillow Dec 20, 2017

danawillow Dec 20, 2017

danawillow Dec 20, 2017

danawillow Dec 20, 2017

sl1pm4t commented Dec 20, 2017

danawillow left a comment

ghost commented Mar 30, 2020


		# google\_container\_cluster

		Get info about a cluster within GCE from its name and zone.

New datasource: google_container_cluster #740

New datasource: google_container_cluster #740

Conversation

sl1pm4t commented Nov 15, 2017

sl1pm4t commented Nov 20, 2017

danawillow commented Nov 20, 2017

sl1pm4t commented Nov 20, 2017

sl1pm4t commented Nov 21, 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

sl1pm4t Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

sl1pm4t commented Dec 11, 2017

sl1pm4t commented Dec 19, 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

sl1pm4t commented Dec 20, 2017

danawillow left a comment

Choose a reason for hiding this comment

ghost commented Mar 30, 2020

sl1pm4t Nov 22, 2017 •

edited

Loading