azurerm_virtual_machine to use Lists instead of Sets
#426
Conversation
tombuildsstuff
force-pushed
the
vm-sets-to-list
branch
from
0b2ebe4 to
96ff9a8
Compare
tombuildsstuff
changed the title
azurerm_virtual_machine to use Lists instead of Setsazurerm_virtual_machine to use Lists instead of Sets
|
Existing tests (and some of the new ones) pass:
|
|
|
||
| func TestAccAzureRMVirtualMachine_reorderedDisks(t *testing.T) { | ||
| var beforeVm compute.VirtualMachine | ||
| //var beforeVm, afterVm compute.VirtualMachine |
There was a problem hiding this comment.
didn't realise I'd pushed this last comma sighs I'll remove it :)
| resourceName := "azurerm_virtual_machine.test" | ||
| ri := acctest.RandInt() | ||
| config := testAccAzureRMVirtualMachine_reorderedDisksBefore(ri, testLocation()) | ||
| //updatedConfig := testAccAzureRMVirtualMachine_reorderedDisksAfter(ri, testLocation()) |
| @@ -1557,3 +1496,53 @@ func findStorageAccountResourceGroup(meta interface{}, storageAccountName string | |||
|
|
|||
| return id.ResourceGroup, nil | |||
| } | |||
|
|
|||
| func resourceArmVirtualMachineStorageOsProfileHash(v interface{}) int { | |||
There was a problem hiding this comment.
Just a note here: the implicit schema.HashResource has been working pretty well for me for most complex sets I've been doing. Unless there's specific rules that this structure needs to be abide by for hashing to remove some factors from the algorithm, you might be better off just using that. Can help get rid of some more code too 🙂
There was a problem hiding this comment.
Yeah I was looking into that, unfortunately there's some fields missing from some of the Hash functions (hence why I had to change some from Set -> List -> Set) - and then just relocated them in the file :(
| testCheckAzureRMVirtualMachineExists(resourceName, &beforeVm), | ||
| ), | ||
| }, | ||
| /* |
There was a problem hiding this comment.
Concur on some of the commented out stuff here - could it just be removed as dead code?
There was a problem hiding this comment.
indeed, removed
| ), | ||
| }, | ||
| /* | ||
| { |
There was a problem hiding this comment.
Hey @tombuildsstuff! Left a few comments on the PR here pertaining to maybe simplification of the existing set hashing and commented out tests... otherwise LGTM!
|
Fixed a breaking change in the Azure API (by making the fields Computed) - Tests pass:
|
|
I just tested the NIC ordering and the problem is still appearing. How/when can I see this fixed? |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
Opening this to keep track of things, this seems fine in the initial testing of Linux - need to do some more extensive testing before proceeding with this
I've removed the failing test
TestAccAzureRMVirtualMachine_windowsLicenseTypebecause it's not possible to make this pass in our subscription (for licensing reasons) - however the test will pass in another subscriptionFixes #103
Fixes #99
Fixes #81
Fixes #72
Fixes #20