[WIP] Convert console_supported? to supports? by kbrock · Pull Request #21990 · ManageIQ/manageiq

kbrock · 2022-07-12T22:11:36Z

part of #21989

Add supports :html5_console to all providers that currently support spice_console, vnc_console or webmks_console. This will remove unnecessary logic that derives it. (A trend we have followed for the past 10 years.
Add supports :console to providers with the same logic. Note: webmks_console was not in this list before.
Decide: :html5_console and :console are now basically identical. Do we need both?
Drop console_supported? method and use supports :console instead.
Discuss: some Providers implement validation logic in supports :console*.
Do we want to make this consistent?
Discuss: Moved logic from :launch_html5_console, :launch_native_console, :launch_vmrc_console to supports :{}_console blocks.
supports :*_console now consistently have blocks checking vm for required attributes.

Notes:

I had expected supported_console? to be more prevalent. Felt like it was defined in a bunch of places, but not really used that much.
Think some of the unsupported messaging changed in the supports for providers changed. Feels like we could normalize these, but it's probably best to make as few changes here as possible to avoid too many changes to the localization strings.
vmware cloud manager states supports :html5_console, but unlike others, doesn't have the supports :*_console.
lenovo states supports :console, but doesn't state the specific types. (same as above)
some enable/disable html5_console support based upon a feature flag or the state of the vm (e.g. archived?). Other providers seem to do this logic in supports :launch_html5_console
supports :html5_console and :console are looking the same

This requires the following PRs in other providers:

all were modified to handle the supports
Put DONE by the ones that also converted validate and launch_

The wording on some of the older ones may be a little different.
Here are the old values:

    supports_not :html5_console  # { _("The web-based HTML5 Console is not supported") }
    supports_not :native_console # { _("VM NATIVE Console not supported") } 
    supports_not :spice_console # { N_("Console not supported") }
    supports_not :vmrc_console   # { _("VMRC Console not supported") }
    supports_not :vnc_console   # { N_("Console not supported") }
    supports_not :webmks_console # { N_("Console not supported") }

I would have preferred that the feature name not be included in the definition. It feels like "not supported" is good enough for the text, but unsure of the implications in the ui.

agrare · 2022-07-14T19:24:01Z

-    supports :vmrc_console do
-      unsupported_reason_add(:vmrc_console, _("VMRC Console not supported")) unless console_supported?('VMRC')
+    supports :console do
+      unless supports?(:spice_console) || supports?(:vnc_console)


Realize this was pulled from https://github.com/ManageIQ/manageiq/pull/21990/files#diff-1cd4edb18c242ba5e1f5795da3f815cde970e5e9672cdd7d96b1a101fe7c465fL1679 but I wonder why webmks isn't an option here

I was doing it blind but I think that is a good idea

TODO: Add to provider classes supports :console

this changes vmware cloud manager and no others

agrare · 2022-07-14T19:24:25Z

-    supports :native_console do
-      unsupported_reason_add(:native_console, _("VM NATIVE Console not supported")) unless console_supported?('NATIVE')
+    supports :html5_console do
+      unless %w[vnc_console webmks_console spice_console].any? { |type| supports?(type) }


This is begging for supports_any? 😉

Huh. most of the cases are actually supports_all?

For supports_all, I had wanted to add supports :html5_console => [:vnc_console ...] but it added too much mental complexity in the definition of supports and unsupported_reason(:a) || unsupported_reason(:b) is easy enough to read.

with the new merge, we really only want unsupported_reason for the all case, not sure how to type that unsupported_any_reason

But for the any case, I guess supports_any would work great. will think on that

TODO: add to providers: supports :html5_console

miq-bot · 2022-07-21T16:23:28Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2023-02-27T00:07:11Z

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.

kbrock · 2023-03-02T01:33:48Z

If this seems like a good idea, maybe we should just add the supports into the children classes and not do a block here at all

miq-bot · 2023-03-02T14:37:36Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2023-03-06T00:07:37Z

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.

kbrock · 2023-03-07T18:00:10Z

The consensus is we want to remove any dynamic calculations we are doing here
and just add the following to the various provider implementations:

grep for console_supported spice

  supports :console # ovirt has this dependent upon html5 enabled
  supports :html5_console
  supports :native_console # currently a thing
  supports :vnc_console # ibm (currently console_supports)

kbrock · 2023-03-08T00:57:09Z

update:

old version was a POC
removed blocks from supports :console, :html_console
(in child providers) webmks now linked to console (in addition to html_console) - only changed vmware

If you like the direction this is headed, then we need a big cross repo test.

- moving all supports into provider classes - moving :console from vm_or_template to vm/operations with the rest of them. - explicitly stating html5_console and console - native_console had half in supports and half in console_supported. so less of a change - dropping console_supported?

kbrock · 2023-03-08T01:03:49Z

update:

missed the change to vm/operations. the tests were still green. concerning but assuming (hoping) the provider tests would have been red for those instead?

miq-bot · 2023-03-08T01:07:30Z

Checked commit kbrock@4a4f57d with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🍰

Fryguy · 2023-03-15T19:21:34Z

-    supports :native_console do
-      _("VM NATIVE Console not supported") unless console_supported?('NATIVE')
-    end
+    supports_not :console


I saw a comment about ovirt, but what exactly is this one? @agrare?

Fryguy · 2023-03-15T19:23:57Z

Hard to tell from this PR, but if the actual supported features are changing, then I think we need to update the service UI as well. cc @DavidResende0 as he's looking at those specifically as we speak.

kbrock · 2023-03-23T13:48:09Z

WIP:

this got too big
seems the concepts of the various supported are confusing

NEXT STEPS:
Go through the providers mentioned and copy validates logic into supports blocks.

Some providers have conditional logic in the supports :vnc_console and others don't even have supports :vnc_console and only use supports_console?('vnc')

miq-bot · 2023-06-26T00:00:08Z