Use SupportsFeatureMixin for querying VM for "resize" operation #9683

gauravaradhye · 2016-07-08T15:59:37Z

Purpose:
SupportsFeatureMixin provides convenient way to specify if a particular provider has support for a feature. This pull request removes validate_resize (old usage) and makes checking feature support by calling supports_resize possible.

Relevant previous PRs:
Similar changes in:

Steps for Testing/QA

Rspec output:
bundle exec rspec spec/models/manageiq/providers/openstack/cloud_manager/vm_spec.rb

Randomized with seed 23687
.........................

Finished in 3.97 seconds (files took 5.69 seconds to load)
25 examples, 0 failures

Randomized with seed 23687

durandom · 2016-07-08T17:16:15Z

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

@@ -1,4 +1,5 @@
 class ManageIQ::Providers::CloudManager::Vm < ::Vm
+  include SupportsFeatureMixin


This is already included in VmOrTemplate parent class

durandom · 2016-07-08T17:24:04Z

Good first shot :)
Are there any other usages of validate_resize in the code?

gauravaradhye · 2016-07-08T17:50:38Z

Thanks! There are no usages of validate_resize but some similar methods such as validate_resize_confirm and validate_resize_revert. Should I go ahead removing those too?

durandom · 2016-07-08T18:28:50Z

validate_resize_confirm and validate_resize_revert.

looks like those are only used by the service models. So, no, dont touch them.

But could you change this line to expose :supports_resize?

durandom · 2016-07-14T07:41:31Z

app/models/vm_or_template.rb

@@ -187,6 +187,8 @@ class VmOrTemplate < ApplicationRecord
  alias_method :parent_cluster, :ems_cluster
  alias_method :owning_cluster, :ems_cluster

+  supports_not :resize, :reason => _("Resize Operation is not available for VM or Template.")


as this is unlikely to be called on VmOrTemplate and its not really a reason other than it's unsupported, I would drop the reason.

durandom · 2016-07-14T07:49:14Z

@gauravaradhye could you limit this PR to refactor only the resize operation?
That means, we keep validate_vm_control until it's not used anymore and remove it in its own PR.
This might sound a bit complictated, but makes the refactoring a) much safer b) easier to backport.
E.g. if we need to backport only the resize refactoring, we only need to cherry-pick this PR.

Also, please address the rubocop issues. It's good to run rubocop before committing 😄

gauravaradhye · 2016-07-14T13:49:52Z

@durandom yes makes sense. I will update it with required changes. Also, do we have any document for coding guidelines for dev other than checking with rubocop?

durandom · 2016-07-14T14:07:40Z

@gauravaradhye https://github.com/ManageIQ/guides thats all I know of

durandom · 2016-07-14T18:25:50Z

@gauravaradhye please rebase the PR and squash the commits into one

durandom · 2016-07-14T18:29:27Z

app/models/manageiq/providers/openstack/cloud_manager/vm/resize.rb

+    supports :resize do
+      msg = validate_vm_control
+      unsupported_reason_add(:resize, msg[1]) unless msg.nil?
+      unsupported_reason_add(:resize, _("The Instance cannot be resized, current state has to be active or shutoff.")) \


dont wrap lines with \
if they are too long use

unless ... add end

durandom · 2016-07-14T18:33:23Z

...mation_engine/service_models/miq_ae_service_manageiq-providers-openstack-cloud_manager-vm.rb

@@ -6,7 +6,7 @@ class MiqAeServiceManageIQ_Providers_Openstack_CloudManager_Vm < MiqAeServiceVmC
    expose :resize_confirm, :override_return => nil
    expose :resize_revert,  :override_return => nil

-    expose :validate_resize
+    expose :supports_resize?


@gmcculloug I guess for backwards compatability we should still have validate_resize available?
can we do so by adding this?

def validate_resize {:available => supports_resize?, :message => unsupported_reason(:resize)} end

in what context are those methods evaluated?

@durandom That's the nice thing about automate, you never really know if/when the methods are used since it is referenced from user code. 😦 This is pretty new stuff (PR #7922 was merged in April), I do not know of anyone using it.

The suggested solution of adding the validate_resize method would work.

Note: If it helps you can also rename the backend method and redirect automate to the name method using :method => <new_method_name>.
lib/miq_automation_engine/service_models/miq_ae_service_vm_or_template.rb#L19

@gmcculloug thanks, I think we go with adding the wrapper method. 👍 for pointing out the alternative.

@gauravaradhye so, please add this wrapper and remove the expose call altogether

@durandom So should I keep the expose validate_resize statement as it is and change the validate_resize method body to internally use supports_resize?

keep expose: supports_resize?

add the method from the first comment

gauravaradhye · 2016-07-15T19:10:55Z

Confirmed there are no old comments with mentioning "validate_resize". No code changes needed.

durandom · 2016-07-18T09:34:54Z

@gauravaradhye if the automate thing is fixed, this is good to go

gauravaradhye · 2016-07-18T19:14:24Z

Included the validate_resize method in cloud_manager/vm.rb and kept the expose :supports_resize? statement as it is.

Did not include validate_resize method back in app/models/manageiq/providers/openstack/cloud_manager/vm/resize.rb because the base class cloud_manager/vm.rb should take care of it.

durandom · 2016-07-19T09:41:39Z

Did not include validate_resize method back in app/models/manageiq/providers/openstack/cloud_manager/vm/resize.rb because the base class cloud_manager/vm.rb should take care of it.

Why? This is supposed to be removed?

gauravaradhye · 2016-07-19T13:29:38Z

@durandom I have kept validate_resize method in only base class as it would be necessary for automate code that might be calling it. If it is called on subclasses, then the base class method will be called which will internally call the supports_resize method of subclass.

durandom · 2016-07-19T19:55:50Z

I have kept validate_resize method in only base class as it would be necessary for automate code that might be calling it.

This compatibility layer should be in the automate code. miq_ae_service_manageiq-providers-openstack-cloud_manager-vm.rb

gauravaradhye · 2016-07-20T15:38:10Z

@durandom Moved code as required.

durandom · 2016-07-20T19:50:43Z

ok, also please address the comments in app/models/manageiq/providers/openstack/cloud_manager/vm/resize.rb

miq-bot · 2016-07-20T21:06:34Z

Checked commits gauravaradhye/manageiq@4e76048~...ae98632 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
7 files checked, 0 offenses detected
Everything looks good. 👍

chessbyte · 2016-07-20T21:53:50Z

@durandom will merge after your 👍

durandom · 2016-07-21T08:45:07Z

👍 looks great. @chessbyte merge?

@miq-bot add_labels darga/no

durandom · 2016-07-21T12:44:07Z

🎉 @gauravaradhye congrats for your second merged PR, keep them rolling in 😄

himdel · 2018-03-05T16:49:02Z

app/models/manageiq/providers/openstack/cloud_manager/vm/resize.rb

+  included do
+    supports :resize do
+      unsupported_reason_add(:resize, unsupported_reason(:control)) unless supports_control?
+      unless %w(ACTIVE SHUTOFF).include?(raw_power_state)


supports_resize? is never true now, because %w(ACTIVE SHUTOFF) is ["ACTIVE", "SHUTOFF"] and not [ACTIVE, SHUTOFF] which would (actually still not) evaluate to the right %w(on off) array which corresponds to the actual values :).

But [ACTIVE, SHUTOFF] doesn't work within supports..., it won't find the constant.

So... I suggest fixing this to just say %w(on off) and not relying on provider-specific constants :).

@gauravaradhye , @durandom Can you take care of this please? :)

(created ManageIQ/manageiq-providers-openstack#246)

EDIT: never mind, I was wrong, sorry

@himdel - sorry, neither @gauravaradhye nor me are working on the codebase anymore. I'm sure one of the openstack provider maintainers can tackle that. I'm still avail for questions :)

Ah, thanks for the heads up :) don't worry about it, the issue is there, and if it stays there I'll just create a PR one of those days ;). Good luck :)

chessbyte changed the title ~~[WIP] Use SupportsFeatureMixin for querrying VM for resize operation~~ [WIP] Use SupportsFeatureMixin for querying VM for resize operation Jul 8, 2016

chessbyte added the pluggability/providers label Jul 8, 2016

chessbyte assigned durandom Jul 8, 2016

chessbyte added wip technical debt labels Jul 8, 2016

gauravaradhye force-pushed the validateResizeChanges branch from 3f3aa10 to 74d0f32 Compare July 8, 2016 16:30

durandom reviewed Jul 8, 2016
View reviewed changes

durandom reviewed Jul 14, 2016
View reviewed changes

gauravaradhye changed the title ~~[WIP] Use SupportsFeatureMixin for querying VM for resize operation~~ Use SupportsFeatureMixin for querying VM for "resize" operation Jul 14, 2016

durandom reviewed Jul 14, 2016
View reviewed changes

gauravaradhye force-pushed the validateResizeChanges branch from f85ec04 to ed91fc9 Compare July 14, 2016 18:31

durandom reviewed Jul 14, 2016
View reviewed changes

gauravaradhye force-pushed the validateResizeChanges branch from ed91fc9 to d75d3e9 Compare July 14, 2016 18:43

gauravaradhye mentioned this pull request Jul 14, 2016

Replacing validate_evacuate method by supports :evacuate #9833

Merged

chessbyte removed the wip label Jul 14, 2016

gauravaradhye force-pushed the validateResizeChanges branch from d75d3e9 to 82b4737 Compare July 18, 2016 19:12

Use SupportsFeatureMixin for querrying VM for resize operation

4e76048

gauravaradhye force-pushed the validateResizeChanges branch from 82b4737 to 4e76048 Compare July 20, 2016 15:35

Replaced validate_vm_control by supports_control?

ae98632

miq-bot added the darga/no label Jul 21, 2016

chessbyte merged commit c4271de into ManageIQ:master Jul 21, 2016

chessbyte added this to the Sprint 44 Ending Aug 1, 2016 milestone Jul 21, 2016

gauravaradhye deleted the validateResizeChanges branch July 23, 2016 23:18

This was referenced Oct 6, 2016

refactor validate_suspend to supports :suspend #11735

Closed

refactor validate_pause to supports :pause #11736

Closed

refactor validate_clone to supports :clone #11737

Closed

refactor validate_publish to supports :publish #11738

Closed

himdel reviewed Mar 5, 2018

View reviewed changes

himdel mentioned this pull request Mar 5, 2018

The Instance cannot be resized, current state has to be active or shutoff. ManageIQ/manageiq-providers-openstack#246

Closed

kbrock mentioned this pull request Aug 30, 2022

comment the supports feature code ManageIQ/manageiq-automation_engine#505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SupportsFeatureMixin for querying VM for "resize" operation #9683

Use SupportsFeatureMixin for querying VM for "resize" operation #9683

gauravaradhye commented Jul 8, 2016 •

edited

Loading

durandom Jul 8, 2016

durandom commented Jul 8, 2016

gauravaradhye commented Jul 8, 2016

durandom commented Jul 8, 2016

durandom Jul 14, 2016

durandom commented Jul 14, 2016

gauravaradhye commented Jul 14, 2016

durandom commented Jul 14, 2016

durandom commented Jul 14, 2016

durandom Jul 14, 2016

durandom Jul 14, 2016

gmcculloug Jul 15, 2016

durandom Jul 18, 2016

gauravaradhye Jul 18, 2016

durandom Jul 19, 2016

gauravaradhye commented Jul 15, 2016

durandom commented Jul 18, 2016

gauravaradhye commented Jul 18, 2016 •

edited

Loading

durandom commented Jul 19, 2016

gauravaradhye commented Jul 19, 2016

durandom commented Jul 19, 2016

gauravaradhye commented Jul 20, 2016

durandom commented Jul 20, 2016

miq-bot commented Jul 20, 2016

chessbyte commented Jul 20, 2016

durandom commented Jul 21, 2016

durandom commented Jul 21, 2016

himdel Mar 5, 2018 •

edited

Loading

durandom Mar 5, 2018

himdel Mar 5, 2018

		@@ -1,4 +1,5 @@
		class ManageIQ::Providers::CloudManager::Vm < ::Vm
		include SupportsFeatureMixin

Use SupportsFeatureMixin for querying VM for "resize" operation #9683

Use SupportsFeatureMixin for querying VM for "resize" operation #9683

Conversation

gauravaradhye commented Jul 8, 2016 • edited Loading

Steps for Testing/QA

Choose a reason for hiding this comment

durandom commented Jul 8, 2016

gauravaradhye commented Jul 8, 2016

durandom commented Jul 8, 2016

Choose a reason for hiding this comment

durandom commented Jul 14, 2016

gauravaradhye commented Jul 14, 2016

durandom commented Jul 14, 2016

durandom commented Jul 14, 2016

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

Choose a reason for hiding this comment

gauravaradhye commented Jul 15, 2016

durandom commented Jul 18, 2016

gauravaradhye commented Jul 18, 2016 • edited Loading

durandom commented Jul 19, 2016

gauravaradhye commented Jul 19, 2016

durandom commented Jul 19, 2016

gauravaradhye commented Jul 20, 2016

durandom commented Jul 20, 2016

miq-bot commented Jul 20, 2016

chessbyte commented Jul 20, 2016

durandom commented Jul 21, 2016

durandom commented Jul 21, 2016

himdel Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravaradhye commented Jul 8, 2016 •

edited

Loading

gauravaradhye commented Jul 18, 2016 •

edited

Loading

himdel Mar 5, 2018 •

edited

Loading