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.

Sign up for GitHub

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

Jump to bottom

Feature/add snapshot functionalities #362

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

Feature/add snapshot functionalities #362

wants to merge 18 commits into from

Conversation

KevinLoiseau
Copy link
Contributor

@bilal-naeem-confiz @maham-nazir-confiz This is the single PR which contains each functionalities, as you asked.

bilal-naeem-confiz
bilal-naeem-confiz suggested changes Dec 20, 2017
View reviewed changes
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.

Please make the suggested changes. Also, add an integration test for this feature.

lib/fog/azurerm/models/compute/snapshot.rb Outdated
end

def destroy(async = false)
service.delete_snapshot(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.

In case of asynchronous calls, you will have to modify the response to return a Fog::AsyncResponse object. You can see it here

lib/fog/azurerm/models/compute/snapshots.rb Outdated
module Compute
class AzureRM
# This class is giving implementation of all/list, get and
# check existence for snapshots of managedisk.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in managedisk. Should be managed disk.

lib/fog/azurerm/requests/compute/create_or_update_snapshot.rb Outdated
snapshot.name = snapshot_params[:name]
snapshot.type = SNAPSHOT_PREFIX
snapshot.location = snapshot_params[:location]
snapshot.tags = snapshot_params[:tags] if snapshot.tags.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be unless instead of if

lib/fog/azurerm/requests/compute/delete_snapshot.rb
class AzureRM
# Real class for Compute Request
class Real
def delete_snapshot(resource_group_name, snapshot_name, async = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this request is called asynchronously, then the response return should be as done here

test/models/compute/test_snapshot.rb Outdated
end
end

def test_destroy_async_method_true_response
Copy link
Contributor

Choose a reason for hiding this comment

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

This method will be updated once you update your delete method for asynchronous calls.

test/requests/compute/test_delete_snapshot.rb Outdated
end
end

def test_delete_managed_disk_async
Copy link
Contributor

Choose a reason for hiding this comment

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

This method will be updated once you update your delete method for asynchronous calls.

test/requests/compute/test_list_snapshots_by_rg.rb Outdated
@@ -0,0 +1,24 @@
require File.expand_path '../../test_helper', __dir__

# Test class for List Snapshots by Ressource Group Request
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in Resource

@KevinLoiseau
Copy link
Contributor Author

KevinLoiseau commented Jan 30, 2018 •
edited
Loading

@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.

@stiller-leser
Copy link

What's the status of this PR? Would really like to see that integrated. Happy to help if there are any ToDos left.

@bilal-naeem-confiz
Copy link
Contributor

@stiller-leser @KevinLoiseau
Let me go through this PR one more time and then I'll merge it.

@KevinLoiseau
Copy link
Contributor Author

@bilal-naeem-confiz Have you got some news on this pull request ?
Thanks in advance.

@stiller-leser
Copy link

For now I created a fork here: https://github.com/stiller-leser/fog-azure-rm that contains the changes. Would be awesome to see this PR merged though.

@stiller-leser
Copy link

stiller-leser commented Nov 19, 2018 •
edited
Loading

@KevinLoiseau do you happen to have an example on how to actually create a snapshot? This is missing in the documentation of the README.

EDIT:

Nevermind, got it to work:

@connection.create_or_update_snapshot({
        name: snapshot_name,
        location: location,
        resource_group_name: resource_group,
        creation_data: {
          create_option: "Copy",
          source_uri: disk.id
        }
})

And to create a disk from the snapshot:


@connection.managed_disks.create(
        name: disk_name,
        location: location,
        resource_group_name: resource_group,
        # See https://docs.microsoft.com/en-us/azure/storage/common/storage-redundancy-lrs
        account_type: 'Standard_LRS',
        creation_data: {
          create_option: 'Copy',
          source_uri: snapshot.id
        },
        disk_size_gb: snapshot.disk_size_gb,
)

@stiller-leser stiller-leser mentioned this pull request Nov 22, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bilal-naeem-confiz bilal-naeem-confiz bilal-naeem-confiz requested changes

Assignees
No one assigned
Labels
enhancement in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KevinLoiseau @stiller-leser @bilal-naeem-confiz @zhitongLBN