-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Read patch_mode in resourceLinuxVirtualMachineRead #14042
Conversation
Should have been included in #13866 |
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.
Hi @atombrella thanks for updating this! Looks like there are some test failures relating to this new property currently:
------- Stdout: -------
=== RUN TestAccLinuxVirtualMachine_authSSH
=== PAUSE TestAccLinuxVirtualMachine_authSSH
=== CONT TestAccLinuxVirtualMachine_authSSH
testcase.go:109: Step 1/2 error: After applying this test step, the plan was not empty.
stdout:
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
~ update in-place
Terraform will perform the following actions:
# azurerm_linux_virtual_machine.test will be updated in-place
~ resource "azurerm_linux_virtual_machine" "test" {
id = "/subscriptions/*******/resourceGroups/acctestRG-211105164807101935/providers/Microsoft.Compute/virtualMachines/acctestVM-211105164807101935"
name = "acctestVM-211105164807101935"
- patch_mode = "ImageDefault" -> null
# (18 unchanged attributes hidden)
# (3 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccLinuxVirtualMachine_authSSH (289.01s)
FAIL
6e1d9f3
to
52a80f7
Compare
Hmm, maybe this should have had Default: string(compute.LinuxVMGuestPatchModeImageDefault),` under the property? |
This change plus the linting correction should fix the tests! 😄 |
52a80f7
to
99c286f
Compare
@catriona-m @katbyte I updated the PR. Hopefully it can be merged now 🎉 |
HI @atombrella, thank you for making the requested changes, but unfortunately it now looks like the test is failing for a different reason:
|
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.
hey @atombrella
Thanks for pushing those changes - taking a look through here we also need to ensure that this field has a value set into the state at all times and that this default value is documented, but if we can fix those up then we should be able to take another look
Thanks!
e5313e8
to
4715c16
Compare
@tombuildsstuff Thanks. I updated it, so the linting passes now. I haven't yet set up my environment to be able to run the acceptance tests. |
I've updated it a bit. Without the 16.04 change, the test wouldn't run (now I can run the acceptance tests locally). I think I'm a bit stuck with this PR :/
|
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.
Hi @atombrella! Looking at this again it seems the test may be resolved by just removing admin_password
from the linuxPatchModeSetting test config altogether 😄 then we can look at merging this.
9ad4c8e
to
3665627
Compare
Without the nil-changes, the test will fail with:
|
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.
Thanks @atombrella, LGTM! 🚀
This functionality has been released in v2.87.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
No description provided.