Add support for `google_container_node_pool` management #669

davidquarles · 2017-11-02T10:32:32Z

Fixes #655.

danawillow · 2017-11-07T23:03:25Z

Hey @davidquarles, are you looking for any sort of feedback right now or just claiming your spot in fixing this? Wasn't sure what to make of the [WIP].

davidquarles · 2017-11-08T01:22:45Z

@danawillow Yeah... I apologize. I opened a couple of these PRs and started getting pulled in other directions shortly after, but I can circle back to tie this stuff up over the next couple days. This one, as is, is really just a direct translation of the work in #384 at the node-pool level. I prepended [WIP] because I haven't validated any assumptions or written any tests, so I know it's not done, but if you have feedback today, it would certainly be welcome / helpful. Thanks!

danawillow

This looks pretty good so far! Just a few small comments, and I'm looking forward to seeing tests soon :)

danawillow · 2017-11-15T18:10:14Z

google/resource_container_node_pool.go

+		MaxItems: 1,
+		Elem: &schema.Resource{
+			Schema: map[string]*schema.Schema{
+


nit: extra newline

danawillow · 2017-11-15T18:10:22Z

google/resource_container_node_pool.go

+	"management": {
+		Type:     schema.TypeList,
+		Optional: true,
+		Computed: true,


Is this actually computed? (note: it might be! it's just sometimes hard to tell from looking at the api, so I usually ask)

I'm obviously somewhat fuzzy on the exact meaning of computed and when exactly to use it / not use it (especially in the context of new attributes with ForceNew: true being added to resources that are already out in the wild, though this is not that case), if you don't mind giving me a one-time clarification. In this context, the field is always passed back from the API and both values are false if unspecified. Based on my current understanding, I'd think that means the field is optional + computed, but that could be totally incorrect. I can do a bit of testing on my own to get a better grasp.

The way I like to think about Computed is that it doesn't cause a diff if the value is set in state but not in the config. For a simple case, take name. We allow the user to set this field to empty, which means that terraform will generate a name for you, which we then store in state. Since we don't want the next run of terraform plan to show a diff (since you aren't actually trying to set the name to empty), we have name as Computed so that when it's empty, nothing happens.

So in this case, if management is always returned from the api, then it will always get set in state via the flatten fn, so Computed is indeed the correct choice. A good example of the opposite is autoscaling- if it isn't enabled, then it doesn't get returned from the API at all (so np.Autoscaling gives nil), meaning we don't want it to get set in state.

The reason I like to double-check that a field needs to beComputed is that this empty-means-no-diff-behavior means that removing the field from your config doesn't set it to the default value, as might be expected. So for example, if we completely remove the autoscaling block, terraform will notice there's a diff between what's in state (autoscaling enabled) and what's in the config (no autoscaling) and will call update appropriately. For management, if (say) auto_repair is set to true in state, and then I remove the entire management block in the hopes of "removing" management, terraform won't see that as a diff and so nothing will be updated.

The way I usually handle this when developing is to just set nothing to Computed and see what fails in testing. In this particular case, since Management is returned from the API, I think you should leave it as Computed but set the two properties to be Default: false. That way, if they are set to true in the schema and then removed, terraform will see that it needs to set it back to false. It'll just work for those individual fields, not the full management block, but I think it's still better than nothing.

I hope that makes sense! It's taken me a long time to get to my current level of understanding so feel free to ask any more questions you might have :)

Rad! Thanks so much for the great, thorough explanation. That makes complete sense. I'll make that change and re-test now.

danawillow · 2017-11-15T18:14:01Z

google/resource_container_node_pool.go

@@ -415,6 +460,37 @@ func nodePoolUpdate(d *schema.ResourceData, meta interface{}, clusterName, prefi
 		}
 	}

+	if d.HasChange(prefix + "management") {
+


nit: extra newline

danawillow · 2017-11-15T18:14:48Z

google/resource_container_node_pool.go

+		if v, ok := d.GetOk(prefix + "management"); ok {
+			managementConfig := v.([]interface{})[0].(map[string]interface{})
+			management.AutoRepair = managementConfig["auto_repair"].(bool)
+			management.AutoUpgrade = managementConfig["auto_upgrade"].(bool)


since this api is json:omitempty, you'll also need to add management.ForceSendFields = []string{"AutoRepair", "AutoUpgrade"} so you can change values to false

danawillow · 2017-11-15T18:17:47Z

google/resource_container_node_pool.go

+			return waitErr
+		}
+
+		log.Printf("[INFO] Updated management in Node Pool %s", d.Id())


I'd use npName instead of d.Id() since this function gets called for google_container_node_pool resources as well as google_container_cluster ones.

davidquarles · 2017-11-17T06:46:15Z

$ make testacc TEST=./google TESTARGS='-run=_withManagement'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=_withManagement -timeout 120m
=== RUN   TestAccContainerNodePool_withManagement
--- PASS: TestAccContainerNodePool_withManagement (1114.72s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	1114.747s

danawillow · 2017-11-17T19:19:19Z

google/resource_container_node_pool.go

+	"management": {
+		Type:     schema.TypeList,
+		Optional: true,
+		Computed: true,


The way I like to think about Computed is that it doesn't cause a diff if the value is set in state but not in the config. For a simple case, take name. We allow the user to set this field to empty, which means that terraform will generate a name for you, which we then store in state. Since we don't want the next run of terraform plan to show a diff (since you aren't actually trying to set the name to empty), we have name as Computed so that when it's empty, nothing happens.

So in this case, if management is always returned from the api, then it will always get set in state via the flatten fn, so Computed is indeed the correct choice. A good example of the opposite is autoscaling- if it isn't enabled, then it doesn't get returned from the API at all (so np.Autoscaling gives nil), meaning we don't want it to get set in state.

The reason I like to double-check that a field needs to beComputed is that this empty-means-no-diff-behavior means that removing the field from your config doesn't set it to the default value, as might be expected. So for example, if we completely remove the autoscaling block, terraform will notice there's a diff between what's in state (autoscaling enabled) and what's in the config (no autoscaling) and will call update appropriately. For management, if (say) auto_repair is set to true in state, and then I remove the entire management block in the hopes of "removing" management, terraform won't see that as a diff and so nothing will be updated.

The way I usually handle this when developing is to just set nothing to Computed and see what fails in testing. In this particular case, since Management is returned from the API, I think you should leave it as Computed but set the two properties to be Default: false. That way, if they are set to true in the schema and then removed, terraform will see that it needs to set it back to false. It'll just work for those individual fields, not the full management block, but I think it's still better than nothing.

I hope that makes sense! It's taken me a long time to get to my current level of understanding so feel free to ask any more questions you might have :)

danawillow · 2017-11-17T19:22:19Z

google/resource_container_node_pool_test.go

+var testAccContainerNodePool_withManagement = fmt.Sprintf(`
+resource "google_container_cluster" "cluster" {
+	name = "tf-cluster-nodepool-test-%s"
+	zone = "us-central1-a"


nit: mind running terraform fmt on this file to align all the =s?

Unfortunately vim-terraform doesn't autodetect nested strings within go files. 😛
Note that If i visually select each block and pipe out to terraform fmt (i.e. :'<,'>!terraform fmt -), I end up with two leading spaces, rather than tabs, along with various other format changes for pretty much every single nested block within the file, which is actually what I'd expect from terraform fmt. Should we do this for all the tests uniformly in a separate PR? I'll push a separate commit doing so for the individual file here, just to illustrate, which I can discard (or not).

:set expandtab|:set noexpandtab and :GoFmt both appear to have no effect, either

Ah yeah, we don't really have a consistent story on whether we should use tabs or spaces for configs within tests. I don't really have an opinion either way, I just want the nice alignment :)
I do think though that we should either use spaces for all tests (in a given file) or tabs for all, so for now it probably makes sense to discard the commits and align by hand

Yeah, that seems sane. I'm generally not a huge fan of owning hundreds of lines of code I don't necessarily understand for the sake of formatting. 😛

danawillow · 2017-11-17T19:25:06Z

google/resource_container_node_pool_test.go

+
+var testAccContainerNodePool_withManagement = fmt.Sprintf(`
+resource "google_container_cluster" "cluster" {
+	name = "tf-cluster-nodepool-test-%s"


I think you intended on this test checking update, but since the name is changing from the basic test, it'll actually delete and recreate the cluster/np

I didn't actually intend to test updates, but it totally should. I'll change it, re-test, and circle back shortly.

davidquarles · 2017-11-18T05:05:03Z

make testacc TEST=./google TESTARGS='-run=_withManagement'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=_withManagement -timeout 120m
=== RUN   TestAccContainerNodePool_withManagement
--- PASS: TestAccContainerNodePool_withManagement (671.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	671.029s

danawillow · 2017-11-21T20:56:23Z

Looks good, thanks @davidquarles!

* add support for `google_container_node_pool` management (sans-tests) * [review] add tests, docs, general cleanup * add docs * [review] amend test to check updates and terraform fmt * test updates, make nested management fields non-computed

…hashicorp#669) Signed-off-by: Modular Magician <magic-modules@google.com>

ghost · 2020-03-30T14:13:05Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

davidquarles force-pushed the np-management branch from 4f2851b to 1946401 Compare November 2, 2017 10:36

paddycarver added the waiting-response label Nov 8, 2017

danawillow removed the waiting-response label Nov 8, 2017

danawillow reviewed Nov 15, 2017

View reviewed changes

danawillow added the waiting-response label Nov 15, 2017

davidquarles added 3 commits November 16, 2017 21:05

add support for google_container_node_pool management (sans-tests)

becccdf

[review] add tests, docs, general cleanup

043ff50

add docs

9d99eb6

davidquarles force-pushed the np-management branch from 1946401 to 9d99eb6 Compare November 17, 2017 06:13

davidquarles changed the title ~~[WIP] add support for google_container_node_pool management~~ Add support for google_container_node_pool management Nov 17, 2017

danawillow reviewed Nov 17, 2017

View reviewed changes

davidquarles force-pushed the np-management branch from a5b1fd7 to fd89e9a Compare November 17, 2017 21:51

[review] amend test to check updates and terraform fmt

84e54e1

davidquarles force-pushed the np-management branch from fd89e9a to 84e54e1 Compare November 17, 2017 23:48

test updates, make nested management fields non-computed

f1800ea

danawillow merged commit 34297fe into hashicorp:master Nov 21, 2017

This was referenced Nov 21, 2017

providers/google: Support for Node Auto-Upgrade in google_container_cluster (GKE) #79

Closed

providers/google: Support for Node Auto-Repair in google_container_cluster (GKE) #80

Closed

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Sep 27, 2019

[terraform] Add AuthenticatorGroupsConfig to google_container_cluster (…

2cc07c6

…hashicorp#669) Signed-off-by: Modular Magician <magic-modules@google.com>

davidquarles mentioned this pull request Feb 4, 2020

Add support for management settings in google_container_cluster NAP #5579

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `google_container_node_pool` management #669

Add support for `google_container_node_pool` management #669

davidquarles commented Nov 2, 2017

danawillow commented Nov 7, 2017

davidquarles commented Nov 8, 2017

danawillow left a comment

danawillow Nov 15, 2017

davidquarles Nov 17, 2017

danawillow Nov 15, 2017

davidquarles Nov 17, 2017

danawillow Nov 17, 2017

davidquarles Nov 17, 2017

danawillow Nov 15, 2017

davidquarles Nov 17, 2017

danawillow Nov 15, 2017

davidquarles Nov 17, 2017

danawillow Nov 15, 2017

davidquarles Nov 17, 2017

davidquarles commented Nov 17, 2017

danawillow Nov 17, 2017

danawillow Nov 17, 2017

davidquarles Nov 17, 2017

davidquarles Nov 17, 2017

danawillow Nov 17, 2017

davidquarles Nov 17, 2017

danawillow Nov 17, 2017

davidquarles Nov 17, 2017

davidquarles commented Nov 18, 2017

danawillow commented Nov 21, 2017

ghost commented Mar 30, 2020

Add support for google_container_node_pool management #669

Add support for google_container_node_pool management #669

Conversation

davidquarles commented Nov 2, 2017

danawillow commented Nov 7, 2017

davidquarles commented Nov 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidquarles commented Nov 17, 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

Choose a reason for hiding this comment

davidquarles commented Nov 18, 2017

danawillow commented Nov 21, 2017

ghost commented Mar 30, 2020

Add support for `google_container_node_pool` management #669

Add support for `google_container_node_pool` management #669