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

v2v: Add support for driver ISOs #121

Merged
merged 1 commit into from
Jun 5, 2017
Merged

v2v: Add support for driver ISOs #121

merged 1 commit into from
Jun 5, 2017

Conversation

matobet
Copy link
Contributor

@matobet matobet commented May 29, 2017 •
edited by gmcculloug
Loading

@matobet
Copy link
Contributor Author

matobet commented May 29, 2017

@miq-bot assign @tinaafitz

@matobet
Copy link
Contributor Author

matobet commented May 29, 2017

@miq-bot add_label fine/yes

@matobet
Copy link
Contributor Author

matobet commented May 30, 2017

Depends on ManageIQ/manageiq-automation_engine#31

@matobet matobet mentioned this pull request May 30, 2017
Merged
tinaafitz
tinaafitz reviewed May 30, 2017
View reviewed changes
...t/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_driver_isos.rb
@@ -0,0 +1,46 @@
module ManageIQ
Copy link
Member

Choose a reason for hiding this comment

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

@matobet Can you add a spec test for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tinaafitz done

tinaafitz
tinaafitz reviewed May 30, 2017
View reviewed changes
...te/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/create_vm_import_request.rb
@@ -28,6 +28,7 @@ def main
'cluster_id' => @handle.root['dialog_cluster'],
'storage_id' => @handle.root['dialog_storage'],
'sparse' => @handle.root['dialog_sparse'],
'drivers_iso' => @handle.root['dialog_install_drivers'] && @handle.root['dialog_drivers_iso']
Copy link
Member

Choose a reason for hiding this comment

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

@matobet Can you update the spec test for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tinaafitz done

@tinaafitz
Copy link
Member

@matobet Looks good.
@gmcculloug Please review.

gmcculloug
gmcculloug reviewed Jun 2, 2017
View reviewed changes
...t/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_driver_isos.rb Outdated

def main
values_hash = {}
values_hash[nil] = '-- select drivers ISO from list --'
Copy link
Member

Choose a reason for hiding this comment

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

@matobet Seems like it would be better to not set any default message here. Instead below if you do not have a provider you could provide an message like "-- Select Provider first--". This "select drivers" message would move down into the current else block where you are listing the ISOs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmcculloug done

@gmcculloug
Copy link
Member

@matobet I added a link to the related PR in the ovirt repo for future reference. Please add useful links to the description in future PRs.

@matobet
Copy link
Contributor Author

matobet commented Jun 5, 2017

@gmcculloug sure no problem, I thought having the github automatic comments "PR mentioned from" is enough but sure, I can post the bulk to every PR's description for convenience

@miq-bot
Copy link
Member

miq-bot commented Jun 5, 2017

Checked commit matobet@1b8b673 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. 🍰

@gmcculloug gmcculloug merged commit ec6ee1f into ManageIQ:master Jun 5, 2017
@gmcculloug gmcculloug added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 5, 2017
simaishi pushed a commit that referenced this pull request Jun 12, 2017
@gmcculloug @simaishi
Merge pull request #121 from matobet/v2v-driver-automate
6704283 
v2v: Add support for driver ISOs
(cherry picked from commit ec6ee1f)

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

Fine backport details:

$ git log -1
commit 670428321e60f7c40ba9b643a615187a7fce2474
Author: Greg McCullough <[email protected]>
Date:   Mon Jun 5 08:54:07 2017 -0400

    Merge pull request #121 from matobet/v2v-driver-automate
    
    v2v: Add support for driver ISOs
    (cherry picked from commit ec6ee1fc5a81635aa41556e4d13b2f9e54cd2ff1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1459996

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

@tinaafitz tinaafitz tinaafitz left review comments

@gmcculloug gmcculloug gmcculloug left review comments

Assignees

@tinaafitz tinaafitz

Labels
enhancement fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matobet @tinaafitz @gmcculloug @miq-bot @simaishi