[WIP] Convert console_supported? to supports? #21990

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

part of #21989

First:

Added supports :html5_console to providers instead of deriving it in main. (it is supported if any of the following is supported: spice_console, vnc_console, webmks_console.
Added supports :console to providers with the same logic. Note: before, webmks_console was not in this list.
:html5_console and :console are now basically identical.
Droped console_supported? method.
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 probably best to make as few changes here to not produce too many changes the he 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 his 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 it if the feature name were not 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

app/models/vm/operations.rb

-    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

app/models/vm/operations.rb

-    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

app/models/vm/operations.rb

-    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

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot · 2023-10-16T00:00:03Z

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot · 2024-01-22T00:00:39Z

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

miq-bot · 2024-04-29T00:00:31Z

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

kbrock added the wip label Jul 12, 2022

kbrock requested review from agrare and Fryguy as code owners July 12, 2022 22:11

kbrock mentioned this pull request Jul 12, 2022

Use supports in more instances #21989

Open

18 tasks

agrare reviewed Jul 14, 2022

View reviewed changes

miq-bot added the unmergeable label Jul 21, 2022

miq-bot added the stale label Feb 27, 2023

miq-bot closed this Feb 27, 2023

kbrock reopened this Mar 2, 2023

miq-bot closed this Mar 6, 2023

kbrock removed the stale label Mar 7, 2023

kbrock reopened this Mar 7, 2023

kbrock changed the title ~~[WIP] drop supports_console and instead use supports~~ [WIP] convert console_supported? to supports? Mar 7, 2023

kbrock added refactoring and removed wip unmergeable labels Mar 8, 2023

kbrock assigned agrare Mar 8, 2023

kbrock changed the title ~~[WIP] convert console_supported? to supports?~~ Convert console_supported? to supports? Mar 8, 2023

kbrock force-pushed the supports_console branch from b74d988 to d65a610 Compare March 8, 2023 00:55

kbrock force-pushed the supports_console branch from d65a610 to 4a4f57d Compare March 8, 2023 01:02

kbrock mentioned this pull request Mar 8, 2023

[WIP] convert console_supported? to supports? ManageIQ/manageiq-api#1209

Open

Fryguy reviewed Mar 15, 2023

View reviewed changes

kbrock changed the title ~~Convert console_supported? to supports?~~ [WIP] Convert console_supported? to supports? Mar 16, 2023

kbrock added the wip label Mar 16, 2023

miq-bot added the stale label Jun 26, 2023

kbrock removed the stale label Jul 15, 2023

miq-bot added the stale label Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Convert console_supported? to supports? #21990

[WIP] Convert console_supported? to supports? #21990

kbrock commented Jul 12, 2022 •

edited

Loading

agrare Jul 14, 2022

kbrock Jul 21, 2022

kbrock Mar 7, 2023 •

edited

Loading

kbrock Mar 7, 2023

agrare Jul 14, 2022

kbrock Jul 21, 2022

kbrock Mar 7, 2023

miq-bot commented Jul 21, 2022

miq-bot commented Feb 27, 2023

kbrock commented Mar 2, 2023

miq-bot commented Mar 2, 2023

miq-bot commented Mar 6, 2023

kbrock commented Mar 7, 2023

kbrock commented Mar 8, 2023 •

edited

Loading

kbrock commented Mar 8, 2023

miq-bot commented Mar 8, 2023

Fryguy Mar 15, 2023

Fryguy commented Mar 15, 2023

kbrock commented Mar 23, 2023

miq-bot commented Jun 26, 2023

miq-bot commented Oct 16, 2023

miq-bot commented Jan 22, 2024

miq-bot commented Apr 29, 2024

[WIP] Convert console_supported? to supports? #21990

Are you sure you want to change the base?

[WIP] Convert console_supported? to supports? #21990

Conversation

kbrock commented Jul 12, 2022 • edited Loading

agrare Jul 14, 2022

Choose a reason for hiding this comment

kbrock Jul 21, 2022

Choose a reason for hiding this comment

kbrock Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kbrock Mar 7, 2023

Choose a reason for hiding this comment

agrare Jul 14, 2022

Choose a reason for hiding this comment

kbrock Jul 21, 2022

Choose a reason for hiding this comment

kbrock Mar 7, 2023

Choose a reason for hiding this comment

miq-bot commented Jul 21, 2022

miq-bot commented Feb 27, 2023

kbrock commented Mar 2, 2023

miq-bot commented Mar 2, 2023

miq-bot commented Mar 6, 2023

kbrock commented Mar 7, 2023

kbrock commented Mar 8, 2023 • edited Loading

kbrock commented Mar 8, 2023

miq-bot commented Mar 8, 2023

Fryguy Mar 15, 2023

Choose a reason for hiding this comment

Fryguy commented Mar 15, 2023

kbrock commented Mar 23, 2023

miq-bot commented Jun 26, 2023

miq-bot commented Oct 16, 2023

miq-bot commented Jan 22, 2024

miq-bot commented Apr 29, 2024

kbrock commented Jul 12, 2022 •

edited

Loading

kbrock Mar 7, 2023 •

edited

Loading

kbrock commented Mar 8, 2023 •

edited

Loading