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

Add query parameter to DeleteRole to delete disk based on boolean #238

Merged
merged 2 commits into from
Dec 7, 2015

Conversation

kbxkb
Copy link
Contributor

@kbxkb kbxkb commented Nov 18, 2015

DeleteRole REST API (used to destroy a VM) needs a query parameter to also delete the VHD disk from storage while deleting the VM. This parameter is "comp=media". Documentation is here: https://msdn.microsoft.com/en-us/library/azure/jj157184.aspx

Without this parameter, the DeleteRole() method would only delete the VM but not the disk, thereby leaking storage dollars. Added the parameter, but made it effective only if the boolean argument is true. Thanks.

Context: I am trying to fix hashicorp/terraform#3568. In other words, I am adding the capability to manage multiple VM-s under the same Cloud Service to Terraform. Once this capability is there, we will have to also be able to destroy one VM at a time. As of now, terraform takes the shortcut of deleting the entire cloud service when deleting a VM. That works today as all terraform supports is one VM per cloud service. Hence DeleteRole() method is not called, DeleteDeployment() is called directly. But as part of the fix, I must start calling DeleteRole() to remove individual VM-s. Thus, DeleteRole() must work correctly. Hence this code change. Thanks

…sk from storage while deleting the VM. Added the parameter
…e deleted or retained - as per review comment on this pull request - Azure#237
@msftclas
Copy link

Hi @kbxkb, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Koushik Biswas). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@ahmetb
Copy link
Contributor

ahmetb commented Nov 18, 2015

LGTM. Although this is breaking the API signature, I think we can take it as we've done it many times in the past for management pkg and we're not tracking it in version tags. @paulmey PTAL.

@kbxkb
Copy link
Contributor Author

kbxkb commented Nov 19, 2015

Though Terraform is just one user of azure-sdk-for-go, DeleteRole() is not called from Terraform (yet, before my fixes). There may be other users out there who may get impacted by the API signature modification. Having said that, I do agree with @ahmetalpbalkan that this is a necessary change. Thanks!

@ahmetb
Copy link
Contributor

ahmetb commented Nov 19, 2015

@kbxkb we always suggest other projects to vendor a specific tag/commit.@paulmey PTAL and merge as you see fit.

@kbxkb
Copy link
Contributor Author

kbxkb commented Dec 7, 2015

@paulmey can you please take a look at your convenience, thanks!

@paulmey
Copy link
Member

paulmey commented Dec 7, 2015

LGTM

paulmey added a commit that referenced this pull request Dec 7, 2015
Add query parameter to DeleteRole to delete disk based on boolean
@paulmey paulmey merged commit 98b2c9f into Azure:master Dec 7, 2015
@paulmey
Copy link
Member

paulmey commented Dec 7, 2015

Sorry for the delay and thanks for contributing!

@davem-git
Copy link

What version is this fix in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I am not able to create multiple VMs which belong to the same service
5 participants