provider/google: initial commit for node pool resource #11802

danawillow · 2017-02-08T22:41:56Z

Purposely kept to just required fields for now to keep the PR small.

Will enable further development in this resource, such as is necessary for #8605.

paddycarver

Thanks for working on this, @danawillow! It looks great, just a couple minor things to look at.

paddycarver · 2017-02-18T00:39:18Z

builtin/providers/google/container_operation.go

+			w.Project, w.Zone, w.Op.Name).Do()
+		log.Printf("[DEBUG] Progress of operation %q: %q", w.Op.Name, resp.Status)
+
+		return resp, resp.Status, err


I think if err is not nil, resp may be nil, in which case this will panic.

paddycarver · 2017-02-18T00:42:04Z

builtin/providers/google/resource_container_node_pool.go

+	return &schema.Resource{
+		Create: resourceContainerNodePoolCreate,
+		Read:   resourceContainerNodePoolRead,
+		Delete: resourceContainerNodePoolDelete,


This isn't a requirement, but I'd love to see an Exists function here.

Done. Can you help me now figure out whether there's anything that can be taken out of the other functions now that the Exists one is there?

Absolutely, I'll mark them up if I find them.

paddycarver · 2017-02-18T00:44:59Z

builtin/providers/google/resource_container_node_pool.go

+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,
+			},


It'd be great if we could a name_prefix or something similar, so we can have random strings attached to the end and not have conflicts when using this with create_before_destroy.

paddycarver · 2017-02-18T01:34:54Z

builtin/providers/google/resource_container_node_pool.go

+	project, err := getProject(d, config)
+	if err != nil {
+		return err
+	}


I think we want to use a project_id inline here, so we can take advantage of creating projects in Terraform and using them as the host for node pools.

project_id or project? I'm seeing both: https://github.com/hashicorp/terraform/blob/master/builtin/providers/google/resource_google_project_iam_policy.go#L21 vs https://github.com/hashicorp/terraform/blob/master/builtin/providers/google/resource_google_project.go#L47.

Hm. I don't have a preference either way, and honestly, can see arguments for both:

project is probably a bit more natural and consistent (e.g., we use disk, instance, not disk_id, instance_id)

project is going to sound weird in google_project

project_id leaves us more wiggle room in the future if we need to add/change something and want to avoid BC changes.

Looking into the actual implementation of getProject (I thought it used environment variables...? Maybe I imagined that) I actually don't see anything wrong with it. If we add project to the schema, everything should be fine, and getProject actually seems like the right way to go. Sorry for the misdirection.

In the future we should standardise on project, though I think it best to wait to update google_project as 1) getProject doesn't make sense for that resource and 2) I'm hesitant to deprecate project_id only like 3 releases after introducing it.

Seems reasonable. I added a project field.

paddycarver · 2017-02-18T01:35:52Z

builtin/providers/google/resource_container_node_pool.go

+	op, err := config.clientContainer.Projects.Zones.Clusters.NodePools.Create(project, zone, cluster, req).Do()
+
+	if err != nil {
+		return err


If we could add some context to this error, that would be awesome. Something like "Error creating node pool:" or something.

paddycarver · 2017-02-18T01:38:01Z

builtin/providers/google/resource_container_node_pool.go

+func resourceContainerNodePoolRead(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+
+	project, err := getProject(d, config)


Same here, I think we want project_id in the config, instead.

Nope, I was wrong, this is still just adding a project attribute to the schema.

paddycarver · 2017-02-18T01:41:22Z

builtin/providers/google/resource_container_node_pool.go

+func resourceContainerNodePoolDelete(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+
+	project, err := getProject(d, config)


This should probably be read from the Config.

Still wrong, just need a project property on the resource.

paddycarver

Big change is just adding a project property to the schema, so getProject will work as intended and we can specify projects besides the authenticating one.

I also need to follow up on Exists and make sure I'm telling you the right thing about what is needed/is not needed.

paddycarver · 2017-02-24T22:16:04Z

builtin/providers/google/resource_container_node_pool.go

+	return &schema.Resource{
+		Create: resourceContainerNodePoolCreate,
+		Read:   resourceContainerNodePoolRead,
+		Delete: resourceContainerNodePoolDelete,


Absolutely, I'll mark them up if I find them.

paddycarver · 2017-02-24T22:22:27Z

builtin/providers/google/resource_container_node_pool.go

+	project, err := getProject(d, config)
+	if err != nil {
+		return err
+	}


Hm. I don't have a preference either way, and honestly, can see arguments for both:

project is probably a bit more natural and consistent (e.g., we use disk, instance, not disk_id, instance_id)

project is going to sound weird in google_project

project_id leaves us more wiggle room in the future if we need to add/change something and want to avoid BC changes.

Looking into the actual implementation of getProject (I thought it used environment variables...? Maybe I imagined that) I actually don't see anything wrong with it. If we add project to the schema, everything should be fine, and getProject actually seems like the right way to go. Sorry for the misdirection.

In the future we should standardise on project, though I think it best to wait to update google_project as 1) getProject doesn't make sense for that resource and 2) I'm hesitant to deprecate project_id only like 3 releases after introducing it.

paddycarver · 2017-02-24T22:23:06Z

builtin/providers/google/resource_container_node_pool.go

+func resourceContainerNodePoolRead(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+
+	project, err := getProject(d, config)


Nope, I was wrong, this is still just adding a project attribute to the schema.

paddycarver · 2017-02-24T23:21:37Z

builtin/providers/google/resource_container_node_pool.go

+	nodePool, err := config.clientContainer.Projects.Zones.Clusters.NodePools.Get(
+		project, zone, cluster, name).Do()
+	if err != nil {
+		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 {


I think this should no longer be necessary, as Exists should never let this code execute if the resource doesn't exist. But @radeksimko said something that made me doubt that, so I'm going to follow up with him before I start handing out wrong info.

If that is the case, we (I think) still need somewhere to mark the id as empty. I could do it in Exists but the comments around the declaration of it say The *ResourceData passed to Exists should _not_ be modified. which certainly is a deterrent for doing it there.

Unless I'm terribly mistaken I don't think you need to run d.SetId("") in Exists in order to mark it as gone.
That's what happens automatically if Exists returns false, see https://github.com/hashicorp/terraform/blob/master/helper/schema/resource.go#L237-L243

That said the Exists function will need some extra care because right now it's returning false if it encounters any error (could be 503), so the conditional w/ 404 here should be moved down there and Exists should always properly return genuine error as true, error.

Makes sense, thanks @radeksimko!

paddycarver · 2017-02-24T23:33:30Z

builtin/providers/google/resource_container_node_pool.go

+func resourceContainerNodePoolDelete(d *schema.ResourceData, meta interface{}) error {
+	config := meta.(*Config)
+
+	project, err := getProject(d, config)


Still wrong, just need a project property on the resource.

danawillow · 2017-03-01T21:04:13Z

All right @paddyforan, I think all comments have been resolved at this point. Over to you.

paddycarver

Partial review, noticed you pushed a new commit, so gonna leave this here so I can refresh.

paddycarver · 2017-03-01T17:32:03Z

builtin/providers/google/resource_container_node_pool.go

+					"GOOGLE_PROJECT",
+					"GCLOUD_PROJECT",
+					"CLOUDSDK_CORE_PROJECT",
+				}, nil),


I'd rather see this default to an empty string, honestly; getProject would populate the provider-configured project, if I'm not mistaken.

Huh, for some reason tests were failing when I had it this way before but I just tried again and they passed, so \shrug. Done.

paddycarver

This looks great. 👍 Merge at will.

provider/google: initial commit for node pool resource

* 'master' of github.com:hashicorp/terraform: provider/datadog: Update to datadog_monitor still used d.GetOk (#12497) Check instance is running before trying to attach (#12459) Fix aws_dms_replication_task diff for json with whitespace. (#12380) add "name" to exported attributes (#12483) provider/aws: Adding an acceptance test to for ForceNew on ecs_task_definition volumes provider/aws: (#10587) Changing volumes in ECS task definition should force new revision. provider/aws: Change aws_spot_fleet_request tests to use the correct hash values in test cases Small doc updates (#12165) Improve description of consul_catalog_entry (#12162) provider/aws: Only send iops when creating io1 devices. Fix docs (#12392) provider/google: initial commit for node pool resource (#11802) Fix spurious user_data diffs Properly handle 'vpc_security_group_ids', drop phantom 'security_groups' Default 'ebs_optimized' and 'monitoring' to false

provider/google: initial commit for node pool resource

ragdaj · 2017-05-24T13:56:32Z

Is it possible to set machine type for node pool ?

ghost · 2020-04-12T01:59:56Z

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.

danawillow added new-resource provider/google-cloud labels Feb 8, 2017

danawillow requested a review from paddycarver February 8, 2017 22:41

paddycarver suggested changes Feb 18, 2017

View reviewed changes

danawillow force-pushed the gke-nodepool branch 2 times, most recently from 2add5ae to bdcac4f Compare February 22, 2017 23:17

paddycarver suggested changes Feb 24, 2017

View reviewed changes

danawillow added 3 commits February 27, 2017 14:41

provider/google: initial commit for node pool resource

e8b0143

add exists method, name_prefix, and better error messages

1fde18a

add project field

c92cc9c

danawillow force-pushed the gke-nodepool branch from bdcac4f to c92cc9c Compare February 27, 2017 22:56

move 404 checking

a4ebb36

paddycarver reviewed Mar 1, 2017

View reviewed changes

remove defaultfunc on project

c808269

paddycarver approved these changes Mar 2, 2017

View reviewed changes

danawillow merged commit 3220446 into hashicorp:master Mar 6, 2017

danawillow added a commit that referenced this pull request Mar 6, 2017

provider/google: initial commit for node pool resource (#11802)

b9d284b

provider/google: initial commit for node pool resource

danawillow deleted the gke-nodepool branch March 6, 2017 23:12

yanndegat pushed a commit to yanndegat/terraform that referenced this pull request Mar 13, 2017

provider/google: initial commit for node pool resource (hashicorp#11802)

b472190

provider/google: initial commit for node pool resource

ghost locked and limited conversation to collaborators Apr 12, 2020

provider/google: initial commit for node pool resource #11802

provider/google: initial commit for node pool resource #11802

Conversation

danawillow commented Feb 8, 2017

paddycarver 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

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

paddycarver 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

danawillow commented Mar 1, 2017

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver left a comment

Choose a reason for hiding this comment

ragdaj commented May 24, 2017

ghost commented Apr 12, 2020