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

azurerm_virtual_machine.vhd_uri not picked up in validation #470

Merged
merged 4 commits into from Nov 1, 2017
Merged

azurerm_virtual_machine.vhd_uri not picked up in validation #470

merged 4 commits into from Nov 1, 2017

Conversation

privatwolke
Copy link
Contributor

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.

@privatwolke privatwolke changed the title fixed: issue where azurerm_virtual_machine.vhd_uri would not be picked up in validation azurerm_virtual_machine.vhd_uri not picked up in validation Oct 31, 2017
Copy link
Member

@tombuildsstuff tombuildsstuff left a 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))
Copy link
Member

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)
Copy link
Member

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

Copy link
Contributor Author

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.

@privatwolke
Copy link
Contributor Author

@tombuildsstuff I've updated the branch and I have also converted account_type to account_tier/account_replication_type.

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM :)

@tombuildsstuff
Copy link
Member

Tests pass:

screen shot 2017-11-01 at 14 43 32

@tombuildsstuff tombuildsstuff merged commit c6609f5 into hashicorp:master Nov 1, 2017
tombuildsstuff added a commit that referenced this pull request Nov 1, 2017
@ghost
Copy link

ghost commented Apr 1, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_storage_blobs can't be attached to vms since v.0.9.3
2 participants