-
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
azurerm_virtual_machine.vhd_uri not picked up in validation #470
Conversation
azurerm_virtual_machine.vhd_uri
would not be picked up in validationThere 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 @privatwolke
Thanks for porting this PR over - I've taken a look through and left a couple of minor comments, but this otherwise LGTM :)
Thanks!
CHANGELOG.md
Outdated
|
||
BUG FIXES: | ||
|
||
* `azurerm_virtual_machine` - fix issue where `vhd_uri` would not be picked up in validation ([#62](https://github.com/terraform-providers/terraform-provider-azurerm/issues/62)) |
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.
we tend to fill this in manually, once it's merged (otherwise we get merge conflicts all over the place) - as such would it be possible to revert this file?
ri := acctest.RandInt() | ||
preConfig := testAccAzureRMVirtualMachine_basicLinuxMachine(ri, testLocation()) | ||
prepConfig := fmt.Sprintf(testAccAzureRMVirtualMachine_basicLinuxMachine_destroyVM, ri, ri, ri, ri, ri) | ||
config := fmt.Sprintf(testAccAzureRMVirtualMachine_basicLinuxMachine_storageBlob_attach, ri, ri, ri, ri, ri, ri, ri) |
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.
would it be possible to update these to use the newer-style formatting methods? in particular we now pass in the test location, allowing these tests to be run in different regions. Here's an example of what I'm referring too and the associated formatting method
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.
Sure! I'll update the PR shortly.
@tombuildsstuff I've updated the branch and I have also converted |
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.
LGTM :)
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
This pull request fixes #62 by reusing and updating code by @brandontosch in hashicorp/terraform#13686. I had to fix some test code compared to the original pull request which was created before the provider split. I've also created a changelog entry.
The code builds, tests and runs smoothly for me. Please advise if there are any improvements that I can make to this code to get it release-ready.