🐛 Fix nil pointer if no cluster autoscaling

[bakayolo]

Related to hashicorp/terraform-provider-google#5685

[megan07]

Hi @bappr! Thank you for your contribution. I'm unable to reproduce the error that you're getting, would you be able to give me the configuration you have that is leading to this error? Or add a test that fails before this change, but passes after?
Thanks!

[bakayolo]

Hey @megan07,
So basically, it's on this function that is here https://github.com/terraform-providers/terraform-provider-google-beta/pull/1766/files#diff-1d8fddb33edfcab666a2f3539719ddf3R2722

flattenClusterAutoscaling(a *containerBeta.ClusterAutoscaling)

When a is nil, it's failing.
I checked and unfortunately there is no test already there for that.

FYI, it's not failing when planning but applying.

[megan07]

@bappr, thank you for the clarification! I've spent some time digging into this, and I think my confusion is the fact that a == nil and !a.EnableNodeProvisioning should not be considered equivalent in this conditional. It appears AutoscalingProfile can be set regardless of the value of EnabledNodeProvisioning, which I'm guessing is why it is being set outside that block. However, you're absolutely right that this will cause issues when a is nil.

That being said, I propose we split this into two conditionals (a == nil and a.EnableNodeProvisioning) where the previous fields are set inside the a.EnableNodeProvisioning block, and a == nil returns nil? (I think that's how other nil objects are handled).

Would you like to take a stab at that, or would you like us to make that change?

Thanks for catching this!

[bakayolo]

Hey @megan07
Oh yes you're right. Did not see it was "NodeProvisioning". I thought it was "NodeAutoscaling" and therefore, it did not make much sense. :)
Anyway, I added the fix. FYI, I did not return nil if a == nil since it's not the original behaviour (r["enabled"]=false) and I am afraid it could break something somewhere else.

[megan07]

Looks great, thank you!

[emilymye]

@megan07 sorry to take over, but we've had a couple issues filed - I'm going to go ahead and upstream this one and then merge the PRs in.