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 compute image model #399

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

KevinLoiseau
Copy link
Contributor

@KevinLoiseau KevinLoiseau commented Oct 2, 2018

Hi !

In this PR you will find:

  • Add Image model with destroy and save methods
  • Add Images collection with get and create methods
  • Enhance the create_image request to be able to create image from Virtual Machine or from Managed Disk

UPDATE:

  • Add request to list images
  • Add methods all to images collection

Thanks !

test/models/compute/test_image.rb Outdated Show resolved Hide resolved
test/api_stub/requests/compute/image.rb Show resolved Hide resolved
lib/fog/azurerm/requests/compute/create_image.rb Outdated Show resolved Hide resolved
@KevinLoiseau KevinLoiseau force-pushed the enhancement/add_compute_image_model branch from 8a81edb to b7dc131 Compare October 3, 2018 07:30
@KevinLoiseau KevinLoiseau force-pushed the enhancement/add_compute_image_model branch from d530738 to 8ce3fc9 Compare October 30, 2018 15:37
@maham-nazir-confiz
Copy link
Contributor

maham-nazir-confiz commented Nov 13, 2018

Hi @KevinLoiseau
I have reviewed your PR.
It breaks when trying to create an image from a virtual machine. It throws the exception that

The operation 'Create Image' requires the Virtual Machine 'TestVM-Managed' to be Generalized.

Please take a look at this link => https://docs.microsoft.com/en-us/azure/virtual-machines/linux/capture-image

It mentions all the steps that need to be done before a VM can be captured. Update the PR accordingly.
Add an integration test for this as well.

Thank you.
Regards

@KevinLoiseau
Copy link
Contributor Author

Hi @maham-nazir-confiz
Thank you for your feedback.

I had this error during my tests of this functionnallity but it was not a code's issue; like your link describe you have to "generalized" your dellocated Vm before trying to create an image of it.
For this you have to use this method: https://github.com/fog/fog-azure-rm/blob/master/lib/fog/azurerm/models/compute/server.rb#L127-L130.

I don't think the generalization of the VM should be orchestred by the create_image request, IMHO this is the responsability of the gem user to know prerequisites, and the error raised by the SDK gem is pretty clear. WDYT ?

Regards,
Kevin

@maham-nazir-confiz
Copy link
Contributor

@KevinLoiseau
Yes I think that's fine. The user should first power off and generalize the VM.
I need you to make the following changes in your PR

  • Add the instructions in the docs for the user to first power off and then generalize the VM before creating an image.
  • Add an integration test that tests all the methods (new and previous) for the images
  • Incorporate HoundCI feedback
  • Your PR breaks for the destroy method because it calls the delete_image method that further attaches the suffix -osImage to the image name passed to it. Whatever changes you make to fix this, please ensure that if any other class is using this method as well, it doesn't break for them.

Thank you.
Regards

location: '<Location>',
resource_group_name: '<Resource Group Name>',
managed_disk_id: '<Managed Disk Id>',
os_disk_type: '<Plateform>'
Copy link
Contributor

@abbas-sheikh-confiz abbas-sheikh-confiz Nov 14, 2018

Choose a reason for hiding this comment

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

It should be "".

## List Images

```ruby
images = fog_compute_service..images(resource_group: '<Resource Group Name>').all
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, remove the double dots.

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.

4 participants