Snapshots revert API #13829
Snapshots revert API #13829
Conversation
| @@ -299,7 +299,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) | |||
There was a problem hiding this comment.
why this change here ? do you need to rebase maybe ?
There was a problem hiding this comment.
Up to this point we haven't needed to have a subresource POST action, so this wasn't needed in the previous PR
There was a problem hiding this comment.
I'm confused, we had the delete post action before. we may be missing a test for that, so it was never exercised.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
missing delete post action test ?
There was a problem hiding this comment.
Not sure I understand - is this not it?
There was a problem hiding this comment.
sorry, misread the diffs. yes you have it.
spec/requests/api/snapshots_spec.rb
Outdated
| @@ -374,6 +420,52 @@ | |||
| end | |||
| end | |||
|
|
|||
| describe "POST /api/vms/:c_id/snapshots/:s_id with revert action" do | |||
imtayadeway
force-pushed
the
api/snapshots-revert
branch
from
a7e332e
to
a108a24
Compare
|
Please rebase this PR to incorporate #13846 in order to ensure api.yml integrity ❤️ ❤️ ❤️ |
|
This pull request is not mergeable. Please rebase and repush. |
imtayadeway
force-pushed
the
api/snapshots-revert
branch
from
a108a24
to
d9a8acb
Compare
config/api.yml
Outdated
| @@ -848,6 +848,8 @@ | |||
| - :name: read | |||
| :identifier: cloud_volume_snapshot_view | |||
| :post: | |||
| - :name: revert | |||
| :identifier: cloud_volume_snapshot_revert # doesn't exist yet | |||
There was a problem hiding this comment.
@h-kataria are you able to advise on this? Unfortunately I don't have any development instances that are snapshot-able - is it possible to revert an instance snapshot? Or is that tied into the backup/restore flow?
There was a problem hiding this comment.
/cc @agrare @blomquisg is it possible to revert snapshots on instances ?
There was a problem hiding this comment.
I don't see any cloud providers implementing snapshot revert, @blomquisg correct me if I'm wrong but it might be because they actually snapshot cloud volumes not instances. At least in GCE you can't revert to a snapshot volume, you just create a new instance from the snapshot, not sure if that's the same in AWS/Azure/etc...
imtayadeway
force-pushed
the
api/snapshots-revert
branch
from
d9a8acb
to
97fbd30
Compare
|
@miq-bot rm-label wip |
imtayadeway
force-pushed
the
api/snapshots-revert
branch
from
97fbd30
to
b221e27
Compare
|
Checked commit imtayadeway@b221e27 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
How did the tests go with a physical Vm ? |
|
Any update on this one? |
|
Just tested this with @imtayadeway and it's good, got VM's reverted on Esx successfully. 👍 |
|
LGTM!! 🎵 |
Adds a revert action to the Vms
and InstancesAPIs@miq-bot add-label wip, api, enhancement
@miq-bot assign @abellotti