Conversation
sbernhard
force-pushed
the
delete_iso
branch
2 times, most recently
from
0574b66 to
51bd8c8
Compare
|
Added ostruct and base64 for newer ruby version. Otherwise, the test failed. |
| options | ||
| end | ||
|
|
||
| def destroy_iso(options = {}) |
There was a problem hiding this comment.
Can the option be actually an empty hash?
What would be working ?
There was a problem hiding this comment.
same as for "upload_iso"
There was a problem hiding this comment.
it is "merged" with 'default_options" in destroy_iso_check_options and if certain options are missing, it will thrown an exception. As said, it's the "same" implementation as in upload_iso
| module Vsphere | ||
| class Compute | ||
| class Real | ||
| def destroy_iso_check_options(options) |
There was a problem hiding this comment.
I don't see any delete action in the method. What is it for?
There was a problem hiding this comment.
Oh, I guess it's for destroy_iso.
Then I suggest moving it to private methods and simply renaming it check_options'!
There was a problem hiding this comment.
actually, the structure is the same as upload_iso
| required_options.each do |param| | ||
| raise ArgumentError, "#{required_options.join(', ')} are required" unless options.key? param | ||
| end | ||
| raise Fog::Vsphere::Compute::NotFound, "Datacenter #{options['datacenter']} Doesn't Exist!" unless get_datacenter(options['datacenter']) |
There was a problem hiding this comment.
IMO, these checks should be implemented as a separate method called from the destroy_iso function.
If both exist, we call the API 4 times. That's not good. We can call the api 2x and get the same results.
Plus, the name destroy_iso_check_options does not indicate that it should check the existence of resources; it should focus on options only
There was a problem hiding this comment.
I think this is pretty usual in fog-vsphere. have a look at lib/fog/vsphere/requests/compute/vm_clone.rb
| default_options = { | ||
| 'upload_directory' => 'isos' | ||
| } | ||
| options = default_options.merge(options) |
There was a problem hiding this comment.
Here, we modify the original options by merging default_options.
So we should either rename the method (process_options) or extract the merge into another method or a different place.
There was a problem hiding this comment.
as said, this is the same way as in upload_iso.rb And I don't think its so bad.
sbernhard
force-pushed
the
delete_iso
branch
2 times, most recently
from
8518d28 to
beef2c1
Compare
sbernhard
force-pushed
the
delete_iso
branch
from
beef2c1 to
0943090
Compare
sbernhard
force-pushed
the
delete_iso
branch
from
0943090 to
2b58c84
Compare
sbernhard
force-pushed
the
delete_iso
branch
from
2b58c84 to
ffbb647
Compare
2 participants
No description provided.