[Issue 74] Create var for Cloud-Init Disk Type

[xrayj11]

This change will allow users to specify a cloud_init_disk_type, which determines which type (ide, sata, scsi) of drive is created for Cloud-Init. If no value is given or the variable is not provided, the Cloud-Init drive will be created on the first available ide drive, which was how it operated before. This change should not break any existing configurations that use Cloud-Init, and only provides additional customization to specify a different drive type than the default ide

These changes passed the make test, as well as packer validation after installing the newly packaged plugin and declaring the cloud_init_disk_type var in my hcl file. I have tested this change with the proxmox-iso builder on each disk type (ide, sata, scsi) as well as not providing the var to ensure it will not break existing configurations. Each option resulted in the expected outcome, where the Cloud-Init drive was created with the type specified by the var.

Closes #74 - By giving the option of SCSI for Cloud-Init

[hashicorp-cla]

All committers have signed the CLA.

[lbajolet-hashicorp]

Hi @xrayj11,

The rework looks good to me, however it seems there are failures during test, also the file should be linted/goimported so the CI is happy, may I ask you to fix those before we can merge?

[xrayj11]

Hey @lbajolet-hashicorp, I've been trying to figure out the issue with the failed all_options_with_cloud-init test, and I think I've narrowed it down. Although we have a case to default to ide when CloudInitDiskType is blank in the Prepare function of config.go, the test is not using it. This means it's going into the Run function with a blank/undefined value and triggering the default case, and erroring out. When I define a valid CloudInitDiskType here the test passes: https://github.com/hashicorp/packer-plugin-proxmox/blob/main/builder/proxmox/common/step_finalize_template_config_test.go#L106-L112 Could we do that, or is there another way we should be testing this since the value can be changed/defaulted by config.go? Also, I've fixed the formatting/goimport issues, they'll be added once we figure out the test.

[lbajolet-hashicorp]

Ah I see @xrayj11, in this case I would suggest either of the two alternatives:

1. Add a call to Prepare before calling Run here: this way only valid configs will reach Run. This is more in line with what we expect from normal plugin usage, as such it cannot hurt imho. This could conflict with other tests though, so I would advise not doing this if this becomes a problem for other test cases.
2. Add a CloudInitDiskType to the config, so it is not empty when executing Run in this case. Given this option is supposed to be vetted/set by Prepare, if the aim of the test is only to test Run in a valid case, this may be a valid change.

I'll let you pick which approach seems more appropriate to you, feel free to ping me when you've settled on either!

[xrayj11]

I looked into trying to call Prepare in the test(s), but it looks like it's going to require a bit of a rewrite of how all the tests are done. I do agree it would be a better implementation of testing since it is more in line with how it actually runs. Since that's out of the scope of this issue, I've just defined CloudInitDiskType: "ide" on any test that uses CloudInit: true. All tests ran good on my side, and in actual packer runs in my environment the plugin still works without defining a CloudInitDiskType, so existing configs will not be affected. Let me know if you see anything else that needs changed, or if this is can be merged. Thanks for the help @lbajolet-hashicorp

[lbajolet-hashicorp]

Looks good to me @xrayj11, thanks for the reroll!

Will merge if/when tests go green