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

Snapshots revert API #13829

Merged
merged 1 commit into from
Feb 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/base_controller/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ def validate_post_api_action_as_subcollection(cname, mname, aname)
return if cname == @req.collection
return if collection_config.subcollection_denied?(@req.collection, cname)

aspec = collection_config.typed_subcollection_actions(@req.collection, cname)
aspec = collection_config.typed_subcollection_actions(@req.collection, cname, @req.s_id ? :subresource : :subcollection)
Copy link
Member

Choose a reason for hiding this comment

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

why this change here ? do you need to rebase maybe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to this point we haven't needed to have a subresource POST action, so this wasn't needed in the previous PR

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, we had the delete post action before. we may be missing a test for that, so it was never exercised.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abellotti in the case of snapshots, the delete action would have used the collection identifier erroneously, which happens to be the same. So there was no way to test this in an integration test.

Copy link
Member

Choose a reason for hiding this comment

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

Got it now, problem is that it worked for delete because it used the delete action's identifier on the subcollection, but we don't implement a bulk revert action on the snapshots subcollection (does not make sense to do so), thus this problem emerged. So I'm good with this fix, Thanks.

return unless aspec

action_hash = fetch_action_hash(aspec, mname, aname)
Expand Down
11 changes: 11 additions & 0 deletions app/controllers/api/subcollections/snapshots.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ def delete_resource_snapshots(parent, type, id, _data)
end
alias snapshots_delete_resource delete_resource_snapshots

def snapshots_revert_resource(parent, type, id, _data)
raise parent.unsupported_reason(:revert_to_snapshot) unless parent.supports_revert_to_snapshot?
snapshot = resource_search(id, type, collection_class(type))

message = "Reverting to snapshot #{snapshot.name} for #{snapshot_ident(parent)}"
task_id = queue_object_action(parent, message, :method_name => "revert_to_snapshot", :args => [id])
action_result(true, message, :task_id => task_id)
rescue => e
action_result(false, e.to_s)
end

private

def snapshot_ident(parent)
Expand Down
2 changes: 2 additions & 0 deletions config/api.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2262,6 +2262,8 @@
- :name: read
:identifier: vm_snapshot_view
:post:
- :name: revert
:identifier: vm_snapshot_revert
- :name: delete
:identifier: vm_snapshot_delete
:delete:
Expand Down
46 changes: 46 additions & 0 deletions spec/requests/api/snapshots_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,52 @@
end
end

describe "POST /api/vms/:c_id/snapshots/:s_id with revert action" do
it "can queue a VM for reverting to a snapshot" do
api_basic_authorize(action_identifier(:vms, :revert, :snapshots_subresource_actions))
ems = FactoryGirl.create(:ext_management_system)
host = FactoryGirl.create(:host, :ext_management_system => ems)
vm = FactoryGirl.create(:vm_vmware, :name => "Alice's VM", :host => host, :ext_management_system => ems)
snapshot = FactoryGirl.create(:snapshot, :name => "Alice's snapshot", :vm_or_template => vm)

run_post("#{vms_url(vm.id)}/snapshots/#{snapshot.id}", :action => "revert")

expected = {
"message" => "Reverting to snapshot Alice's snapshot for Virtual Machine id:#{vm.id} name:'Alice's VM'",
"success" => true,
"task_href" => a_string_matching(tasks_url),
"task_id" => anything
}
expect(response.parsed_body).to include(expected)
expect(response).to have_http_status(:ok)
end

it "renders a failed action response if reverting is not supported" do
api_basic_authorize(action_identifier(:vms, :revert, :snapshots_subresource_actions))
vm = FactoryGirl.create(:vm_vmware)
snapshot = FactoryGirl.create(:snapshot, :vm_or_template => vm)

run_post("#{vms_url(vm.id)}/snapshots/#{snapshot.id}", :action => "revert")

expected = {
"success" => false,
"message" => "The VM is not connected to a Host"
}
expect(response.parsed_body).to include(expected)
expect(response).to have_http_status(:ok)
end

it "will not revert to a snapshot unless authorized" do
api_basic_authorize
vm = FactoryGirl.create(:vm_vmware)
snapshot = FactoryGirl.create(:snapshot, :vm_or_template => vm)

run_post("#{vms_url(vm.id)}/snapshots/#{snapshot.id}", :action => "revert")

expect(response).to have_http_status(:forbidden)
end
end

describe "POST /api/vms/:c_id/snapshots/:s_id with delete action" do
it "can queue a snapshot for deletion" do
api_basic_authorize(action_identifier(:vms, :delete, :snapshots_subresource_actions, :delete))
Copy link
Member

Choose a reason for hiding this comment

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

missing delete post action test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand - is this not it?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, misread the diffs. yes you have it.

Expand Down