drop unsupported_reason_add #901

kbrock · 2024-02-24T03:54:37Z

part of:

Drop Supports' unsupports variable manageiq#22898

Deprecating unsupported_reason_add. Returning the string instead

Note, these are all the same:

unsupported_reason_add(:feature, unsupported_reason(:feature2)) unless supports_feature2?
unsupported_reason_add(:feature, unsupported_reason(:feature2)) unless supports?(:feature2)
unsupported_reason(:feature2) unless supports?(:feature2)
unsupported_reason(:feature2)

kbrock · 2024-02-24T04:11:19Z

I put the redundant return because it was more consistent

agrare · 2024-02-26T17:06:27Z

app/models/manageiq/providers/vmware/cloud_manager/vm.rb

@@ -10,7 +10,7 @@ class ManageIQ::Providers::Vmware::CloudManager::Vm < ManageIQ::Providers::Cloud
  supports :revert_to_snapshot
  supports :reconfigure_disks
  supports :reconfigure_disksize do
-    unsupported_reason_add(:reconfigure_disksize, 'Cannot resize disks of a VM with snapshots') unless snapshots.empty?
+    'Cannot resize disks of a VM with snapshots' unless snapshots.empty?


Should this have gettext wrappers? (We can probably do this in a follow-up since there are a number of these across multiple providers)

Suggested change

'Cannot resize disks of a VM with snapshots' unless snapshots.empty?

_("Cannot resize disks of a VM with snapshots") unless snapshots.empty?

I am unsure if we want to add gettext to all of these.
(also feel like we may want to go towards default values here)

agrare · 2024-02-27T19:53:40Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+      unsupported_reason(:control) ||
+        ("The VM is powered on" if vm_powered_on?)


Suggested change

unsupported_reason(:control) ||

("The VM is powered on" if vm_powered_on?)

if vm_powered_on?

"The VM is powered on"

else

unsupported_reason(:control)

end

agrare · 2024-02-27T19:54:20Z

app/models/manageiq/providers/vmware/infra_manager/host.rb

-    if !supports?(:ipmi)
-      unsupported_reason_add(:start, unsupported_reason(:ipmi))
-    elsif %w[off standby].exclude?(power_state)
-      unsupported_reason_add(:start, _("The Host is not in power state off or standby"))
-    end
+    unsupported_reason(:ipmi) ||
+      (_("The Host is not in power state off or standby") if %w[off standby].exclude?(power_state))


if %w[off standby].exclude?(power_state)) _("The Host is not in power state off or standby") else unsupported_reason(:ipmi) end

agrare · 2024-02-27T19:55:07Z

app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb

+      unsupported_reason(:control) ||
+        (_("The VM is not powered on") unless current_state == "on")


Suggested change

unsupported_reason(:control) ||

(_("The VM is not powered on") unless current_state == "on")

if current_state == "on"

unsupported_reason(:control)

else

_("The VM is not powered on")

end

miq-bot · 2024-02-28T00:05:25Z

Checked commit kbrock@cf716a5 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
10 files checked, 1 offense detected

app/models/manageiq/providers/vmware/infra_manager/host_esx.rb

❗ - Line 223, Col 5 - Style/RedundantReturn - Redundant return detected.

kbrock · 2024-02-28T00:05:28Z

update:

localization x 4 (_())
control(:feature) => if else

jrafanie · 2024-07-01T20:38:37Z

app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb

      end
    end

    supports :shutdown_guest do
-      unsupported_reason_add(:shutdown_guest, unsupported_reason(:control)) unless supports?(:control)
+      reason = unsupported_reason(:control)
+      return reason if reason


@kbrock I'm running some tests with rails 7... probably unrelated... I'm seeing a local jump error here because we're returning from within a block.

[----] E, [2024-07-01T16:07:26.262770#61590:7288] ERROR -- evm: [LocalJumpError]: unexpected return Method:[block (2 levels) in <class:LogProxy>] [----] E, [2024-07-01T16:07:26.262895#61590:7288] ERROR -- evm: /Users/joerafaniello/.gem/ruby/3.1.5/bundler/gems/manageiq-providers-vmware-5c67447bd206/app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb:20:in `block (2 levels) in <module:Guest>' /Users/joerafaniello/Code/manageiq/app/models/mixins/supports_feature_mixin.rb:113:in `instance_eval' /Users/joerafaniello/Code/manageiq/app/models/mixins/supports_feature_mixin.rb:113:in `check_supports' /Users/joerafaniello/Code/manageiq/app/models/mixins/supports_feature_mixin.rb:71:in `supports?' /Users/joerafaniello/Code/manageiq-ui-classic/app/helpers/application_helper/button/generic_feature_button_with_disable.rb:5:in `disabled?' /Users/joerafaniello/Code/manageiq-ui-classic/app/helpers/application_helper/button/basic.rb:54:in `calculate_properties'

I guess we want a structure like

if reason ... elsif current_state == "on" else ... end

Can you take a look?

jrafanie · 2024-07-01T20:41:54Z

app/models/manageiq/providers/vmware/infra_manager/host_esx.rb

-    unsupported_reason_add(:reboot, _("The Host is not connected to an active Provider")) unless has_active_ems?
-    unsupported_reason_add(:reboot, _("The host is not in standby"))                      unless power_state == "standby"
+    return _("The Host is not connected to an active Provider") unless has_active_ems?
+    return _("The host is not in standby")                      unless power_state == "standby"


ah, same problem as below... return from within block

jrafanie · 2024-07-01T20:42:03Z

app/models/manageiq/providers/vmware/infra_manager/host_esx.rb

-    unsupported_reason_add(:exit_maint_mode, _("The Host is not connected to an active Provider")) unless has_active_ems?
-    unsupported_reason_add(:exit_maint_mode, _("The host is not in maintenance mode"))             unless power_state == "maintenance"
+    return _("The Host is not connected to an active Provider") unless has_active_ems?
+    return _("The host is not in maintenance mode")             unless power_state == "maintenance"


These are all the same

jrafanie · 2024-07-01T20:45:55Z

app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb

@@ -3,37 +3,45 @@ module ManageIQ::Providers::Vmware::InfraManager::Vm::Operations::Guest

  included do
    supports :reboot_guest do
-      unsupported_reason_add(:reboot_guest, unsupported_reason(:control)) unless supports?(:control)
+      reason = unsupported_reason(:control)
+      return reason if reason


return from a block behaves in strange ways. What we wanted is to return that value from the block (aka, acted like it is a proc) solution is to not use return Introduced by ManageIQ#901 Solution is to use the pattern used throughout the file

Introduced by ManageIQ#901 Return from a block behaves in strange ways. What we wanted is to return that value from the block (aka, acted like it is a proc) Solution is to change the order of the if block so we can avoid supports? and avoid setting a variable. (and obviously, avoiding the bad return) Error ----- ERROR -- evm: [LocalJumpError]: unexpected return Method:[block (2 levels) in <class:LogProxy>] ERROR -- evm: manageiq-providers-vmware/app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb:20:in `block (2 levels) in <module:Guest>' manageiq/app/models/mixins/supports_feature_mixin.rb:113:in `instance_eval' manageiq/app/models/mixins/supports_feature_mixin.rb:113:in `check_supports' manageiq/app/models/mixins/supports_feature_mixin.rb:71:in `supports?'

Introduced by ManageIQ#901 Return from a block behaves in strange ways. What we wanted is to return that value from the block (aka, acted like it is a proc) Solution is to change the order of the if block so we can avoid supports? and avoid setting a variable. (and obviously, avoiding the bad return) Error ----- ``` ERROR -- evm: [LocalJumpError]: unexpected return Method:[block (2 levels) in <class:LogProxy>] ERROR -- evm: manageiq-providers-vmware/app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb:20: in `block (2 levels) in <module:Guest>' manageiq/app/models/mixins/supports_feature_mixin.rb:113:in `instance_eval' manageiq/app/models/mixins/supports_feature_mixin.rb:113:in `check_supports' manageiq/app/models/mixins/supports_feature_mixin.rb:71:in `supports?' ```

kbrock added the technical debt label Feb 24, 2024

kbrock assigned agrare Feb 24, 2024

kbrock requested review from agrare and Fryguy as code owners February 24, 2024 03:54

kbrock mentioned this pull request Feb 24, 2024

Drop supports_feature? methods ManageIQ/manageiq#22883

Merged

23 tasks

agrare reviewed Feb 26, 2024

View reviewed changes

agrare reviewed Feb 27, 2024

View reviewed changes

drop unsupported_reason_add

cf716a5

kbrock force-pushed the unsupported_supports branch from ffab99e to cf716a5 Compare February 28, 2024 00:04

agrare added the radjabov/yes? label Feb 28, 2024

agrare approved these changes Feb 28, 2024

View reviewed changes

agrare merged commit c4c086f into ManageIQ:master Feb 28, 2024
4 checks passed

agrare removed the radjabov/yes? label Feb 28, 2024

kbrock deleted the unsupported_supports branch February 29, 2024 14:01

jrafanie reviewed Jul 1, 2024

View reviewed changes

This was referenced Jul 1, 2024

remove return from supports blocks #915

Merged

Drop Supports' unsupports variable ManageIQ/manageiq#22898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop unsupported_reason_add #901

drop unsupported_reason_add #901

kbrock commented Feb 24, 2024

kbrock commented Feb 24, 2024

agrare Feb 26, 2024 •

edited

Loading

kbrock Feb 27, 2024

agrare Feb 27, 2024

agrare Feb 27, 2024

agrare Feb 27, 2024

miq-bot commented Feb 28, 2024

kbrock commented Feb 28, 2024

jrafanie Jul 1, 2024

jrafanie Jul 1, 2024

jrafanie Jul 1, 2024

jrafanie Jul 1, 2024

	'Cannot resize disks of a VM with snapshots' unless snapshots.empty?
	_("Cannot resize disks of a VM with snapshots") unless snapshots.empty?

		unsupported_reason(:control) \|\|
		("The VM is powered on" if vm_powered_on?)

		unsupported_reason(:control) \|\|
		(_("The VM is not powered on") unless current_state == "on")

drop unsupported_reason_add #901

drop unsupported_reason_add #901

Conversation

kbrock commented Feb 24, 2024

kbrock commented Feb 24, 2024

agrare Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 28, 2024

kbrock commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Feb 26, 2024 •

edited

Loading