fix: enable new versions of providers to run on old tfstate files #702

[hestiahacker]

This change will automatically convert values from old tfstate files to what is required by the new provider. Specifically, this means converting 0/1 values to true/false, as the code was changed to make that field a boolean value instead of an integer.

This fixes and closes #702

[mleone87]

Not sure about this
Previous to 3.0 tfstate will be unsupported anyway due to removal of deprecated values. As an example, disk params will be removed so correcting this error under the hood will be useless anyway.
I think that we should enforce the correct usage of a bool value and providing the user a clear warning that he should fix the config.

And this will be a learned lesson for breaking changes

[hestiahacker]

Even if this change wouldn't be very useful, could we include this change in a future release so we have an example of how to convert these values in the future? That way next time we change an integer to a boolean (or a similar change), we would have a pattern to mirror.

In cases like this, where we can safely convert types, I think it'd be better to just do so instead of forcing the user to manually update their tfstate file. I agree with you that if there's no way to safely update the tfstate values, we should error out clear instructions on how the user can manually fix up their tfstate file.

Bigger picture

I can understand not accepting old tfstate files for this particular release, since we've had breaking changes in minor updates and this is a major update. It's a good opportunity to start fresh. But in general, it seems like it's important for users to be able to deploy an existing infrastructure with the new version of the software. Otherwise what are people supposed to do when 4.0 is released? Never upgrade? Destroy all their production VMs and start over? Manually update all their .tfstate files?

FWIW, I tested the disk -> disks change and my terraform.tfstate file was updated to set disk to an empty array and disks was populated as expected. There does seem to be some issue which prevents deployment if I don't update the .tf files to use the new convention, but it doesn't look like it is related to the tfstate file being converted over. I'm not going to focus on that since I kinda just want to be done with the 2.x series, which means getting the permissions issues resolved so more people can update to 3.x

[mleone87]

@Tinyblargon need your feedback on this

my opinion is that if we need to silently cover mistakes we need to do it for all fields that can be confusing(e.g. all the boolean/int ones)

[Tinyblargon]

@mleone87 Agreed, we should at least attempt to convert the old way to the new way. This 3.x release might also be a good moment to get rid of of all the ints that should be bools.

As for the permissions issue, I think it's time to move to some dynamic permissions checking system (have a proof of concept lying around somewhere).

[mleone87]

@hestiahacker is checking the int/bools smth that yu can handle on this PR?

[hestiahacker]

Yes, I can do that. Do we have a list of variables where old versions will output integers but the 3.x series is expecting a boolean?

Or were you suggesting that we do the conversion for all boolean fields, even if there never was a version of the provider that would have put an integer there.

I generally prefer to only change code which is problematic in practice and can be tested end to end, but I'm fine with either approach. I just want to make sure there are no pages between us before I go making a bunch of changes.

[hestiahacker]

I've updated this patch to apply cleanly to the refactored code. I also looked for any code that has changed from an integer to a boolean and there doesn't look like there's any other fields that have made this switch in the last 3 years (I didn't look at the code older than that).

There's another person who has reported this issue (#933), so it'd be great if we can get this patch merged in and ideally into the final 3.0.1 release if possible.

If it turns out that I missed some code somewhere which changed format, I'd be happy to file a separate bug and submit a patch to fix it.

[mleone87]

@hestiahacker this code breaks the provider.

StateFunc returns string, not bool but schema_DiskBackup() returns TypeBool making a panic

Did you test the code in manner different from mine? because I'm starting with a fresh vm

Please fix it or revert it

[hestiahacker]

Well this is embarrassing. 😳 I'm not sure what happened here, however it looks like the refactor has put the cast somewhere else and it looks like it's working fine without my patch, so I'll revert it.

As of 2.9.14, there was a bug was where backups could not be disabled. You could set backup=0 on a disk, and the tfstate file would reflect that, but the VM in Proxmox showed the "backup" checkbox as being checked. Setting backup=1 sets the tfstate to 1 and backups are also enabled (explicitly) in the Proxmox UI.

In 3.0.1-rc1, it functions correctly. Omitting the backup field, or setting it to true in your terraform file, results in a true being stored in the tfstate file and the disk in Proxmox will have backup checked. Setting it to false in your terraform file and the disk in Proxmox matches. I mention this in case we want to add it to the release notes.

This caused a ton of confusion for me when I saw the backup value was getting switched from a 0 to a true in the .tfstate file. I thought the new code was converting it incorrectly, and spent hours trying to track that down. It looks like the new code must be pulling the value from Proxmox and geting true, which matches the default value, so it determines no change is needed. Whew!