manageiq icon indicating copy to clipboard operation
manageiq copied to clipboard

[WIP] Convert console_supported? to supports?

Open kbrock opened this issue 3 years ago • 19 comments

part of https://github.com/ManageIQ/manageiq/issues/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:

  • [ ] https://github.com/ManageIQ/manageiq-api/pull/1209 DONE
  • [ ] https://github.com/ManageIQ/manageiq-providers-red_hat_virtualization/pull/24
  • [ ] https://github.com/ManageIQ/manageiq-providers-openstack/pull/841
  • [ ] https://github.com/ManageIQ/manageiq-providers-ovirt/pull/635
  • [ ] https://github.com/ManageIQ/manageiq-providers-ibm_cloud/pull/446
  • [ ] https://github.com/ManageIQ/manageiq-providers-vmware/pull/864 DONE
  • [ ] https://github.com/ManageIQ/manageiq-ui-classic/pull/8687

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.

kbrock avatar Jul 12 '22 22:07 kbrock

Checked commit https://github.com/kbrock/manageiq/commit/b74d988318e22cfe0e3461a380bfeab158e56b92 with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint 3 files checked, 1 offense detected

spec/models/vm/operations_spec.rb

  • [ ] :exclamation: - Line 56, Col 5 - Style/CommentAnnotation - Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.

miq-bot avatar Jul 12 '22 22:07 miq-bot

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

miq-bot avatar Jul 21 '22 16:07 miq-bot

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.

miq-bot avatar Feb 27 '23 00:02 miq-bot

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

kbrock avatar Mar 02 '23 01:03 kbrock

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

miq-bot avatar Mar 02 '23 14:03 miq-bot

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.

miq-bot avatar Mar 06 '23 00:03 miq-bot

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 avatar Mar 07 '23 18:03 kbrock

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.

kbrock avatar Mar 08 '23 00:03 kbrock

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?

kbrock avatar Mar 08 '23 01:03 kbrock

Checked commit https://github.com/kbrock/manageiq/commit/4a4f57d03b5083d43836dc203d33dc9c40b58379 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. :cake:

miq-bot avatar Mar 08 '23 01:03 miq-bot

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.

Fryguy avatar Mar 15 '23 19:03 Fryguy

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')

kbrock avatar Mar 23 '23 13:03 kbrock

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 avatar Jun 26 '23 00:06 miq-bot

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 avatar Oct 16 '23 00:10 miq-bot

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 avatar Jan 22 '24 00:01 miq-bot