Skip to content
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 state migration for vm.vcpus #120

Merged

Conversation

pascal-hofmann
Copy link
Contributor

@pascal-hofmann pascal-hofmann commented Feb 8, 2022

The type change from string to float for vm.vcpus introduced with #92 breaks existing state if vcpus in the existing state is "":

│ Error: Failed to decode resource from state

│ Error decoding "netbox_virtual_machine.this" from previous state: a number is required

This PR bumps the schema version for resourceNetboxVirtualMachine to 1 and introduces a StateUpgrader that will gracefully upgrade the state, even if the value already is a float.

Further reading:

Copy link
Collaborator

@fbreckle fbreckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

Thank you very much for this great PR.
Note that this is the first time I come in contact with state migrations, so bear with me on this one :)
I noticed that you do not throw errors under any circumstances. Is that a best practice pattern to not block the workflow for the user? The type signature of StateUpgradeFunc explicitly allows returning errors.

PS: The website link in your GitHub profile is unreachable ;)

@pascal-hofmann
Copy link
Contributor Author

pascal-hofmann commented Feb 9, 2022

This is my first state migration too, but I think handling it gracefully is the way to go. Otherwise users would have to edit and fix state manually.

The website link in your GitHub profile is unreachable ;)

This is a known issue. 😬🙈

@fbreckle fbreckle merged commit 4e30192 into e-breuninger:master Feb 9, 2022
@pascal-hofmann pascal-hofmann deleted the add-vcpu-state-migration branch July 27, 2022 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants