-
Notifications
You must be signed in to change notification settings - Fork 358
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
[WIP] convert console_supported? to supports? #8687
Conversation
Checked commit kbrock@c7e7637 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint app/helpers/application_helper/button/vm_html5_console.rb
|
Not sure if the cop is valid, but ignoring as it is basically what was there before |
%w[vnc webmks spice].any? { |type| @record.send(:console_supported?, type) } | ||
@record.supports?(:html5_console) |
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.
Wow is this really it?
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 probably rewrite https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/vm_vmrc_console.rb and https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/vm_native_console.rb to be more generic (aka drop the vendor == vmware stuff)
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.
Ok, I read change them to just use "supports?"
Do we have an opinion on whether to change the supports to add some of the other "this vm is on" kind of logic that we are putting in the validates?
Are we thinking we should drop that validates method all together? (I'm suggesting it)
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
This pull request has been automatically closed because it has not been updated for at least 3 months. Feel free to reopen this pull request if these changes are still valid. Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
part of ManageIQ/manageiq#21990
I could have sworn that there was a
@record.supports?(@feature)
in a button, but couldn't find something immediately obvious