-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for google_container_node_pool
management
#669
Conversation
4f2851b
to
1946401
Compare
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]. |
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good so far! Just a few small comments, and I'm looking forward to seeing tests soon :)
MaxItems: 1, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
"management": { | ||
Type: schema.TypeList, | ||
Optional: true, | ||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually computed? (note: it might be! it's just sometimes hard to tell from looking at the api, so I usually ask)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rad! Thanks so much for the great, thorough explanation. That makes complete sense. I'll make that change and re-test now.
@@ -415,6 +460,37 @@ func nodePoolUpdate(d *schema.ResourceData, meta interface{}, clusterName, prefi | |||
} | |||
} | |||
|
|||
if d.HasChange(prefix + "management") { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this api is json:omitempty
, you'll also need to add management.ForceSendFields = []string{"AutoRepair", "AutoUpgrade"}
so you can change values to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return waitErr | ||
} | ||
|
||
log.Printf("[INFO] Updated management in Node Pool %s", d.Id()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
1946401
to
9d99eb6
Compare
google_container_node_pool
managementgoogle_container_node_pool
management
|
"management": { | ||
Type: schema.TypeList, | ||
Optional: true, | ||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
var testAccContainerNodePool_withManagement = fmt.Sprintf(` | ||
resource "google_container_cluster" "cluster" { | ||
name = "tf-cluster-nodepool-test-%s" | ||
zone = "us-central1-a" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: mind running terraform fmt
on this file to align all the =
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:set expandtab
|:set noexpandtab
and :GoFmt
both appear to have no effect, either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😛
|
||
var testAccContainerNodePool_withManagement = fmt.Sprintf(` | ||
resource "google_container_cluster" "cluster" { | ||
name = "tf-cluster-nodepool-test-%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't actually intend to test updates, but it totally should. I'll change it, re-test, and circle back shortly.
a5b1fd7
to
fd89e9a
Compare
fd89e9a
to
84e54e1
Compare
|
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 <[email protected]>
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! |
Fixes #655.
See also: #79, #384, #633.