Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix enabling Power operations for a template #5128

Merged

Conversation

hstastna
Copy link

@hstastna hstastna commented Jan 7, 2019

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1634713
https://bugzilla.redhat.com/show_bug.cgi?id=1655477

What:
This PR fixes the problem about the Power operations which were enabled for a template. And Power operations don't make sense for a template. They should be enabled only for VMs.

Steps to reproduce:

  1. Go to Compute > Infrastructure > VMs > VMs & Templates accordion > All VMs & Templates
  2. Select some template (not VM!) by checking its checkbox
  3. In the toolbar, choose Power > Power On or any other Power action
    => flash message about the start (for example) which was initiated successfully appears and it shouldn't

Before:
start_before

After:
start_after

Note:
I've removed appropriate power actions from untestable actions array in testable_action method which did not make sense to be there. Now the power actions should work properly for VMs and Templates. I've tested power operations for VMs and everything works as usually. And for Templates, the right flash message appears.

@hstastna
Copy link
Author

hstastna commented Jan 7, 2019

@miq-bot add_label bug, hammer/yes, blocker

@mzazrivec
Copy link
Contributor

@hstastna The current CI failures seem related.

@hstastna hstastna changed the title Fix enabling Power operations for a template [WIP] Fix enabling Power operations for a template Jan 9, 2019
@miq-bot miq-bot added the wip label Jan 9, 2019
@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch from 52dc0a9 to fe275bb Compare January 10, 2019 13:09
@martinpovolny
Copy link
Member

So you are removing reboot_guest stop start suspend reset shutdown_guest from the list of the vm_infra_untestable_actions.

@romanblanco added that exceptions for some reason. I am not sure why. I assume that the records_support_feature? would not for for those as it was not supposed by the backend/core.

We need to check what toolbars include these actions. Make a list of pairs entity (such as Instance, VM, Image, ...), action, that can be called (from the buttons below app/helpers/application_helper/toolbar/ ).

For each such pair (that can be called) we must make sure that the code works.

I cannot think about any other reason why @romanblanco added the exceptions (and I dimly remember that there was some problem with checking the availability of those actions on the backend via supports_* or [do not remember the 2nd mechanism]).

If all the combination are ok, we can merge this. W/o this thorough testing we cannot be sure that we are not breaking something here.

@martinpovolny
Copy link
Member

Seems that due to the test if controller == "vm_infra" on line https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/application_controller/ci_processing.rb#L644 the number of toolbars that need to be checked is limited.

However there will be toolbars for nested entities and these need to be checked too.

@hstastna
Copy link
Author

hstastna commented Jan 10, 2019

It looks like it is only about x_vm_center and vm_infras_center toolbars, if we talk about vm_infra controller. I haven't found any other toolbars related to Power operations and the controller. And for all nested entities, the controller is different, not vm_infra.
I think that the fix should be ok. I haven't found any problems with that anywhere.

And I've found couple of bugs while testing, but unrelated to this PR, for example:

  1. go to Compute > Infra > Datastores
  2. click on some datastore to display its details page
  3. click on Managed VMs in the table in the details page (it is required to have some VMs there, not 0)
  4. select some VM(s) by checking its/their checkbox(es)
  5. choose any action under Power operations or Lifecycle > Retire selected items
    =>
[----] F, [2019-01-10T18:50:18.179377 #27579:2aef7da40a28] FATAL -- : Error caught: [ActiveRecord::RecordNotFound] Can't access selected records
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/mixins/checked_id_mixin.rb:60:in `find_records_with_rbac'
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/application_controller/ci_processing.rb:620:in `generic_button_operation'
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/application_controller/ci_processing.rb:405:in `retirevms_now'
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/application_controller/ci_processing.rb:959:in `process_vm_buttons'
/home/hstastna/manageiq/manageiq-ui-classic/app/controllers/storage_controller.rb:93:in `button'

Maybe some other operations will not work properly there. And maybe the BZ for this already exists.

@martinpovolny
Copy link
Member

Ok, will you remove the "WIP label and fix the specs so that we can move this one forward? Thx!

@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch 3 times, most recently from e2443fb to b197195 Compare January 22, 2019 11:28
@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch from 1f22bf8 to 09b7f3a Compare January 28, 2019 12:36
@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch 4 times, most recently from 11179cc to 2a84f8c Compare February 7, 2019 10:50
@hstastna hstastna changed the title [WIP] Fix enabling Power operations for a template Fix enabling Power operations for a template Feb 7, 2019
@hstastna
Copy link
Author

hstastna commented Feb 7, 2019

@skateman I think you can review the specs ;) Thank you!

@@ -9,7 +9,7 @@
controller.instance_variable_set(:@protect_tree, OpenStruct.new(:name => "name"))

MiqRegion.seed
EvmSpecHelper.create_guid_miq_server_zone
_, _, @zone = EvmSpecHelper.create_guid_miq_server_zone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what about not doing this, and just adding something like:

Zone.my_zone

Maybe the method name is not the right one, but I'm pretty sure there's a method to retrieve the current zone.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that this was not necessarry. Thank you, David.

@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch from 2a84f8c to 9e0707c Compare February 11, 2019 12:45
@hstastna
Copy link
Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Feb 11, 2019
@skateman
Copy link
Member

I like it now, but I don't see any tests that cover the power operations for the template. Can you please point them out for me, or if they're not there add them?

@hstastna
Copy link
Author

hstastna commented Feb 12, 2019

The 2nd commit takes care of it but not directly so probably this is not exactly what you expect and maybe I should add very similar specs as there are already for VMs, for vm_infra controller.

@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch from 9e0707c to 681ccc1 Compare February 12, 2019 10:23
@skateman
Copy link
Member

@hstastna please do so

@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch 2 times, most recently from 70bbc36 to 11144d5 Compare February 12, 2019 15:07
@hstastna hstastna force-pushed the Power_dropdown_enabled_for_template branch from 11144d5 to 0643df5 Compare February 13, 2019 11:01
@miq-bot
Copy link
Member

miq-bot commented Feb 13, 2019

Checked commits hstastna/manageiq-ui-classic@74b2cf8~...0643df5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Seal of Approval

@mzazrivec mzazrivec self-assigned this Feb 18, 2019
@mzazrivec mzazrivec added this to the Sprint 105 Ending Feb 18, 2019 milestone Feb 18, 2019
@mzazrivec mzazrivec merged commit 8bf2ad4 into ManageIQ:master Feb 18, 2019
simaishi pushed a commit that referenced this pull request Mar 6, 2019
@simaishi
Copy link
Contributor

simaishi commented Mar 6, 2019

Hammer backport details:

$ git log -1
commit c0dff2f40c665c4f42438f94f16e4e968127c8d6
Author: Milan Zázrivec <[email protected]>
Date:   Mon Feb 18 08:12:19 2019 +0100

    Merge pull request #5128 from hstastna/Power_dropdown_enabled_for_template
    
    Fix enabling Power operations for a template
    
    (cherry picked from commit 8bf2ad4efb7acd3e50b9ac0c9b822eca9ae129eb)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686012
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants