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

Enhance/enable managed disk deletion async #347

Conversation

KevinLoiseau
Copy link
Contributor

Hi,

This little enhance permite to destroy a managed disk asynchronously, according to azure-sdk method (see: https://github.com/Azure/azure-sdk-for-ruby/blob/84fc2ecb6a4d9fd07bfed0182896901f8bf9df6b/management/azure_mgmt_compute/lib/2017-03-30/generated/azure_mgmt_compute/disks.rb#L240).

Result:

client.managed_disks.get('rg', 'disk_name').destroy(true)
=> true

@maham-nazir-confiz
Copy link
Contributor

@KevinLoiseau
Hi, thanks for submitting this.
Can you please update the unit tests for managed disk deletion as done here?

def test_delete_virtual_machine_success

@KevinLoiseau KevinLoiseau force-pushed the enhance/enable_managed_disk_deletion_async branch from 2a4beb1 to d631952 Compare December 8, 2017 10:14
@KevinLoiseau
Copy link
Contributor Author

Copy link
Contributor

@bilal-naeem-confiz bilal-naeem-confiz left a comment

Choose a reason for hiding this comment

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

Make the changes suggested by following what has been done in server methods.

def destroy
service.delete_managed_disk(resource_group_name, name)
def destroy(async = false)
service.delete_managed_disk(resource_group_name, name, async)
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to check if the request is an asynchronous request or not and return the response accordingly. In case of a synchronous request the response will return as is. In case of an asynchronous request you will need to convert the response into a Fog::AsyncResponse object and return that. You can find how to do that here

@@ -4,11 +4,15 @@ module Compute
class AzureRM
# Real class for Compute Request
class Real
def delete_managed_disk(resource_group_name, disk_name)
def delete_managed_disk(resource_group_name, disk_name, async)
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether the call sent is an asynchronous call or not and return the response accordingly. Check the delete method for server here.

end
end

def test_destroy_method_can_take_params_async
Copy link
Contributor

Choose a reason for hiding this comment

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

Update this unit test to reflect the changes you make above. Again, you can reference server unit tests for better understanding.

@KevinLoiseau KevinLoiseau force-pushed the enhance/enable_managed_disk_deletion_async branch from d631952 to 2e271c8 Compare January 30, 2018 09:51
end

def test_destroy_method_can_take_params_async

Choose a reason for hiding this comment

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

Extra empty line detected at method body beginning.

end

def test_destroy_method_can_take_params_async

Choose a reason for hiding this comment

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

Extra empty line detected at method body beginning.

end

def test_destroy_method_can_take_params_async

Choose a reason for hiding this comment

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

Extra empty line detected at method body beginning.

@KevinLoiseau KevinLoiseau force-pushed the enhance/enable_managed_disk_deletion_async branch from 2e271c8 to 541958c Compare January 30, 2018 09:52
@KevinLoiseau
Copy link
Contributor Author

KevinLoiseau commented Jan 30, 2018

@bilal-naeem-confiz Excuse the tardly reply. Your requests are been done.

@KevinLoiseau
Copy link
Contributor Author

@bilal-naeem-confiz can we have some news about this pull request ?
Thanks in advance.
If you are ready for this I will rebase it

@bilal-naeem-confiz
Copy link
Contributor

@KevinLoiseau sorry for the late response. The PR looks good. I can see that there is a merge conflict in the test_delete_managed_disk file. If you can fix that conflict, I'll merge your changes.

@KevinLoiseau KevinLoiseau force-pushed the enhance/enable_managed_disk_deletion_async branch from 541958c to e6cffc3 Compare June 13, 2018 09:35
@KevinLoiseau
Copy link
Contributor Author

@bilal-naeem-confiz Thank you ! The conflict is fixed, up to you.

@bilal-naeem-confiz bilal-naeem-confiz merged commit 42ca817 into fog:master Jun 13, 2018
@bilal-naeem-confiz
Copy link
Contributor

Looks good to me @KevinLoiseau. Thanks for your contribution. Really appreciate it.

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

Successfully merging this pull request may close these issues.

4 participants