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.

Sign up for GitHub

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

Jump to bottom

VM Transform not working from a provider page #2757

Merged
merged 3 commits into from
Nov 18, 2017
Merged

VM Transform not working from a provider page #2757

merged 3 commits into from
Nov 18, 2017

Conversation

lgalis
Copy link
Contributor

@lgalis lgalis commented Nov 16, 2017 •
edited
Loading

Move the vm_transform action to a mixin

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1514032

Steps for Testing/QA

Steps to Reproduce:

  1. Browse CFME UI -> Compute -> Infrastructure -> Providers
  2. Select VMware
  3. click on VMs
  4. Select lifecycle -> Transform tagged VMs to RHV.

With this PR, the dialog opens from ems_infra:

screenshot from 2017-11-17 01-54-53

@lgalis lgalis changed the title VM Transform not working from a provider page [WIP] VM Transform not working from a provider page Nov 16, 2017
@miq-bot miq-bot added the wip label Nov 16, 2017
@lgalis
Copy link
Contributor Author

lgalis commented Nov 17, 2017

/cc @martinpovolny

@lgalis lgalis changed the title [WIP] VM Transform not working from a provider page VM Transform not working from a provider page Nov 17, 2017
@miq-bot miq-bot removed the wip label Nov 17, 2017
@lgalis
Copy link
Contributor Author

lgalis commented Nov 17, 2017

@miq-bot assign @martinpovolny

martinpovolny
martinpovolny reviewed Nov 17, 2017
View reviewed changes
app/controllers/vm_infra_controller.rb Outdated
@@ -2,6 +2,7 @@ class VmInfraController < ApplicationController
include VmCommon # common methods for vm controllers
include VmRemote # methods for VM remote access
include VmShowMixin
include Mixins::Actions::VmActions::Transform
Copy link
Member

Choose a reason for hiding this comment

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

@lgalis , is this line needed? I'd say that the mixing is already included in ApplicationController throught CiProcessing and this controller inherits from Application controller.

martinpovolny
martinpovolny reviewed Nov 17, 2017
View reviewed changes
Copy link
Member

@martinpovolny martinpovolny left a comment

Choose a reason for hiding this comment

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

Looks good. One small thing I commented inline.

@lgalis
Copy link
Contributor Author

lgalis commented Nov 17, 2017

@miq-bot add_label bug, gaprindashvili/yes

@lgalis
Copy link
Contributor Author

lgalis commented Nov 17, 2017

@martinpovolny - addressed your comment. There is one more thing that would need to be addressed - when the dialog is launched from the provider vm list page - the return from the dialog does not include the display paramter. I will include that in a follow up PR as it is a general dialog runner issue, not vm_transform specifc.

@lgalis lgalis force-pushed the fix_vm_transform_from_infra_provider branch from f05549e to 5745233 Compare November 17, 2017 21:19
@martinpovolny
Copy link
Member

Seems the new specs are failing.

@lgalis lgalis force-pushed the fix_vm_transform_from_infra_provider branch from 5745233 to 23e8e87 Compare November 17, 2017 23:11
@lgalis
Copy link
Contributor Author

lgalis commented Nov 17, 2017 •
edited
Loading

@martinpovolny - I pushed a change to the spec by mistake - reverted that

@miq-bot
Copy link
Member

miq-bot commented Nov 17, 2017

Checked commits lgalis/manageiq-ui-classic@41e491f~...23e8e87 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@martinpovolny martinpovolny added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 18, 2017
@martinpovolny martinpovolny merged commit 80fb23f into ManageIQ:master Nov 18, 2017
@lgalis lgalis deleted the fix_vm_transform_from_infra_provider branch November 18, 2017 16:50
simaishi pushed a commit that referenced this pull request Nov 20, 2017
@martinpovolny @simaishi
Merge pull request #2757 from lgalis/fix_vm_transform_from_infra_prov…
a55b882 
…ider

VM Transform not working from a provider page
(cherry picked from commit 80fb23f)

https://bugzilla.redhat.com/show_bug.cgi?id=1515303
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit a55b882f66b5ce7ef949c7bc51fb046ad6016656
Author: Martin Povolny <[email protected]>
Date:   Sat Nov 18 08:48:57 2017 +0100

    Merge pull request #2757 from lgalis/fix_vm_transform_from_infra_provider
    
    VM Transform not working from a provider page
    (cherry picked from commit 80fb23f834f6cec1ff99d8ae6f84b7e91151ae03)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1515303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpovolny martinpovolny martinpovolny left review comments

Assignees

@martinpovolny martinpovolny

Labels
bug compute/infrastructure gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lgalis @martinpovolny @miq-bot @simaishi @dclarizio