-
Notifications
You must be signed in to change notification settings - Fork 896
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
Replaced validate_reboot_guest by supports_reboot_guest #11075
Conversation
@miq-bot add_label wip |
@miq-bot remove_label wip |
@miq-bot add_labels refactoring, "pluggable providers" |
@gauravaradhye Cannot apply the following label because they are not recognized: "pluggable providers" |
validation = instance.send("validate_#{action}") | ||
action_result(validation[:available], validation[:message].to_s) | ||
if instance.respond_to?("supports_#{action}?") | ||
action_result(instance.public_send("supports_#{action}?"), instance.unsupported_reason(action.to_sym)) |
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.
@durandom Not directly related to this PR, but I feel like the unsupported_reason method should accept both strings and symbols, so the caller doesn't have to call .to_sym.
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.
Is there an existing API for features, or is it possible to enhance it for use cases such as these? i.e. If we have to use it with respond_to?
/public_send
that seems like a deficiency. @durandom any thoughts?
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.
@Fryguy if not, I would personally lean towards extracting a service object to handle this stuff....I've been thinking of extracting some services to help lower the total surface area of base_controller.rb
&co.
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.
Is there an existing API for features
@durandom created the SupportsFeatureMixin to move toward a standardized interface for declaring these things. Previously it was a similar but less formalized interface around the is_available?
method. Work is on going to convert from one to the other, and this PR is one of those efforts, but in the middle you are going to have a bit if a hybrid.
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.
Is there an existing API for features, or is it possible to enhance it for use cases such as these? i.e. If we have to use it with respond_to?/public_send that seems like a deficiency
yes, I have this in this PR Then you can just ask every instance/class .supports?(feature)
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.
Do I have to take any action here?
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.
Now that #10917 is merged, you can get rid of one public_send
action_result(instance.supports?(action), instance.unsupported_reason(action.to_sym))
I still have to come up with a query method for checking if a feature is generally queriable.
@durandom Is this good to go? If yes, I will squash the commits. |
validation = vm.send("validate_#{action}") | ||
action_result(validation[:available], validation[:message].to_s) | ||
if vm.respond_to?("supports_#{action}?") | ||
action_result(vm.public_send("supports_#{action}?"), vm.unsupported_reason(action.to_sym)) |
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 here: vm.supports?(action)
@miq-bot add_labels pluggable providers @gauravaradhye it's good, I'll leave it up to you if you want to change |
@gauravaradhye actually I'd leave this as it is, because the So, yes, please squash 😄 |
01daa59
to
f9c4649
Compare
@@ -71,6 +71,7 @@ module SupportsFeatureMixin | |||
:resize => 'Resizing', | |||
:retire => 'Retirement', | |||
:smartstate_analysis => 'Smartstate Analaysis', | |||
:reboot_guest => 'Reboot Guest Operation' |
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.
could you add that in alphabetic order?
Also, as a side note, when adding something to the end of an array or hash, always have a trailing comma, so diffs only will show the changed lines - although thats debatable style :)
e03a68f
to
855cdc7
Compare
The tests passed locally with the recent commit, will wait for the hashy gem fix to be in. |
fb99969
to
8e7c561
Compare
@durandom Made the changes that we discussed, tests passed locally and before squashing commits. Please merge if you think it's good to go! :) |
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.
please remove the two supports_not
calls
extend ActiveSupport::Concern | ||
|
||
included do | ||
supports_not :reboot_guest, :reason => N_("Reboot Guest Operation is not available for the Host.") |
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.
I think we can remove this here, because it unsupported by default
extend ActiveSupport::Concern | ||
|
||
included do | ||
supports_not :reboot_guest, :reason => N_("Reboot Guest Operation is not available for Images/Templates.") |
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 here, it's unsupported by default, so we can remove this
8e7c561
to
544a1a1
Compare
@miq-bot add_label euwe/no |
@gauravaradhye looks good. Do you want to squash before merging? |
8fe1085
to
6ce9954
Compare
@durandom Squashed commits. |
Checked commit gauravaradhye@6ce9954 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
@blomquisg Can you merge this? |
🎉 good work @gauravaradhye - keep the features rollin :) |
@gauravaradhye sorry to resurrect your PR, but can you do the same for the amazon provider and |
@gauravaradhye ignore the prev comment, I did that at ManageIQ/manageiq-providers-amazon#53 |
Thanks @durandom ! |
As advised here: #11677 (comment) we should set the darga and euwe flags @miq-bot add_label euwe/yes, darga/yes |
@miq-bot remove_label darga/yes |
Replaced validate_reboot_guest by supports_reboot_guest (cherry picked from commit 7e9e6b8)
Euwe Backport details: $ git log
commit 789276e71f125ccb5bd5d4b87def9bb9f5b42fc7
Author: Greg Blomquist <blomquisg@gmail.com>
Date: Thu Sep 29 14:14:19 2016 -0400
Merge pull request #11075 from gauravaradhye/refactoring
Replaced validate_reboot_guest by supports_reboot_guest
(cherry picked from commit 7e9e6b8396d63bc220778cea4b812608b5d9f438) |
Purpose or Intent
To use SupportsFeatureMixin instead of AvailabilityMixin, removed instances of validate_reboot_guest method and added supports_rebot_guest method. Changed common methods to check whether given instance supports "supports_*" calls, and call methods accordingly.