-
Notifications
You must be signed in to change notification settings - Fork 897
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
VMs/Snapshots API CRD #13552
VMs/Snapshots API CRD #13552
Conversation
end | ||
|
||
def snapshots_create_resource(parent, _type, _id, data) | ||
parent.snapshots.create!(data.merge(:create_time => Time.zone.now)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti do you know if the create_time
is intended to be set by the user as a required field?
|
||
expected = { | ||
"id" => snapshot.id, | ||
"href" => a_string_matching(snapshots_url(snapshot.id)), # uh-oh.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti bit confused by this behavior. "edit" will return /api/snapshots/:id
(which does not exist), while "create" will return /api/vms/:vm_id/snapshots/:id
. Any thoughts?
fd991ec
to
89e6555
Compare
action_result(true, "Creating snapshot for #{parent.id}", :task_id => task_id) | ||
else | ||
action_result(false, validation[:message]) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer if good path is all protected by rescue:
i.e.
def ...
validation = parent.validate_create_snapshot
raise validation[:message] unless validation[:available]
... do the queuing work
action_result(true, ...)
rescue => err
action_result(false, err.to_s)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, instead of parent.id for the successful return, would be useful to have something similar to vm_ident(), maybe something like '#{ctype} id: #{parent.id} name: #{parent.name}'
action_result(true, "desc", :task_id => task_id) | ||
else | ||
action_result(false, validation[:message]) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same rescue structure suggestion as above.
validation = parent.validate_remove_snapshot(id) | ||
if validation[:available] | ||
task_id = queue_object_action(parent, "summary", :method_name => "remove_snapshot", :args => [id]) | ||
action_result(true, "desc", :task_id => task_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"desc" ? maybe "Deleting snapshot #{id} for ..." same parent signature as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIP 😄
89e6555
to
85acff5
Compare
@miq-bot rm-label wip |
@miq-bot add-label enhancement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @imtayadeway
minor comment change and short of the rubocop changes and tests against physical vm, I'm good with this 🎵
end | ||
end | ||
|
||
describe "POST /api/vms/:c_id/snapshots?action=delete" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update to "POST /api/vms/:c_id/snapshots with delete action" just to clarify that this is not a URL signature (i.e. supporting ?action=delete parameter on POSTs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find!
85acff5
to
522d039
Compare
Checked commits imtayadeway/manageiq@7e47625~...522d039 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
LGTM!! 🎵 |
Here's a naive implementation of the Snapshots API, which I'm submitting for early feedback.@abellotti I'll work on making the implementation more nuanced (i.e. error handling, etc...), but would you like to go over the high-level design together?Adds CRD actions for snapshots of VMs
Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1399526
@miq-bot add-label api, wip
@miq-bot assign @abellotti
Examples
index
show
create
happy path
unhappy path
destroy
happy path
unhappy path