unset disk_size_gb, ignore encryption settings if disabled

[sebastus]

if disk_size_gb = 0 (in HCL), treat it as "unset".

if encryption settings is disabled (but in the HCL), ignore the settings as though they are unset.

[sebastus]

@tombuildsstuff @mbfrahry please review.

[tombuildsstuff]

hey @sebastus

Thanks for this PR / apologies for the delay in reviewing it. This is off to a good start - however can we add an acceptance test to verify this behaviour works as expected (enabling and then disabling encryption)

Thanks!

[tombuildsstuff]

can we add a test covering this scenario (disabling then enabling encryption)?

[sebastus]

@tombuildsstuff I removed the encryption settings changes.

[tombuildsstuff]

@sebastus given we've removed the encryption settings changes from this PR, I'm a little unsure what behavioural changes this will have (given this function is used in VM's, VMSS's and Managed Disks). As such, can we add a test covering an example usage of this updated functionality?

Thanks!

[sebastus]

@tombuildsstuff Allowing for disk_size_gb enables key use cases for managed disk. Example: if copying an existing disk, providing disk_size_gb = 0 causes the copied disk to be the same size as the source. There is a problem with creating a test for this. The DIFF sees that disk_size_gb=0, but the actual size is 10 (or whatever). Is it possible to tell DIFF to ignore this?

[sebastus]

@tombuildsstuff Having examined this further, this goes to the general problem of being unable to 'unset' a property. At this point, it seems that it's impossible to wrap a module around any resource that has optional parameters. I think you said that the core team has this on their backlog. Status of that?

[tombuildsstuff]

hey @sebastus

@tombuildsstuff Allowing for disk_size_gb enables key use cases for managed disk. Example: if copying an existing disk, providing disk_size_gb = 0 causes the copied disk to be the same size as the source. There is a problem with creating a test for this. The DIFF sees that disk_size_gb=0, but the actual size is 10 (or whatever). Is it possible to tell DIFF to ignore this?

So we can add a custom DiffSuppressFunc to do this - but I think it'd be a lot simpler to just make this field Optional + Computed - which would allow the value to be set based on the response from Azure if it's not specified?

@tombuildsstuff Having examined this further, this goes to the general problem of being unable to 'unset' a property. At this point, it seems that it's impossible to wrap a module around any resource that has optional parameters. I think you said that the core team has this on their backlog. Status of that?

@apparentlymart or @jbardin should be able to provide a better answer to this question

Thanks!

[tombuildsstuff]

hey @sebastus

I discussed this IRL with @echuvyrov yesterday - he's imported your changes and added a test for this in PR #800 with the same fix. Given there's a test for this in that PR, I hope you don't mind but I'm going to close this PR in favour of that one.

Thanks!

[ghost]

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!