-
Notifications
You must be signed in to change notification settings - Fork 116
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
Fix #96: During a resource adjustment the VM is not shut down using ACPI #124
Conversation
@PacoDw could you please fix the conflicts? |
…ad of: - should_fail_on_script_failure - enable_script_exec - power_state_mechanism this change represents a better coherency with the API so the above attributes were deprecated, they can be used yet but we recommend to use the new object power_state_mechanism_config
…ig attribute chore: removed comments
@@ -257,7 +257,7 @@ func TestAccNutanixVirtualMachine_PowerStateMechanism(t *testing.T) { | |||
ResourceName: resourceName, | |||
ImportState: true, | |||
ImportStateVerify: true, | |||
ImportStateVerifyIgnore: []string{"disk_list"}, | |||
ImportStateVerifyIgnore: []string{"power_state_mechanism_config", "power_state_mechanism"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?Can't we add those values in the read function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes let change and check them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are in the read function but they depend on a config to be set and they have the same attribute but with different structure, the power_state_mechanism
will be deprecated, so I put them in ImportStateVerifyIgnore array due to a state error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just fix the conflicts otherwise LGTM
@yannickstruyf3 can you confirm if this is still needed since #96 is fixed with the timeout? |
As the corresponding issue is closed, we are closing this PR |
Added:
power_state_mechanism_config
attribute was added, and we can use it as a following:Also, the following Terraform configuration can work with the old attributes that represented the above new configuration:
But now they are deprecated, and we don't recomend using them due to inconsistency and bugs that were fixed with this new change.
Let me know if you have another comment or concern, thank you so much
closes #96