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

Expose validate_conversion_hosts for TransformationPlanRequest #263

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Expose validate_conversion_hosts for TransformationPlanRequest #263

merged 1 commit into from
Oct 30, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2018

This PR is the automation-engine counterpart of ManageIQ/manageiq#18135. It will allow Automate to check the request and deny it if no conversion host is available.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1640816

@ghost
Copy link
Author

ghost commented Oct 26, 2018

@miq-bot add-label transformation, enhancement, hammer/yes

@JPrause
Copy link
Member

JPrause commented Oct 26, 2018

@miq-bot add_label blocker

gmcculloug
gmcculloug reviewed Oct 26, 2018
View reviewed changes
...omation_engine/service_models/miq_ae_service_service_template_transformation_plan_request.rb Outdated
@@ -1,6 +1,7 @@
module MiqAeMethodService
class MiqAeServiceServiceTemplateTransformationPlanRequest < MiqAeServiceServiceTemplateProvisionRequest
expose :source_vms, :association => true
expose :validate_conversion_hosts, :association => true
Copy link
Member

Choose a reason for hiding this comment

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

This is fine but I'm realizing that none of these methods actually return an association so all of the :associations => true should be removed.

Even source_vms (which should likely be renamed at some point) is only returning IDs not actually objects which is what you would expect for an association, and the naming.

@fdupont-redhat Please remove the :association = true for validate_conversion_hosts line and I'll create a follow-up PR to resolve the other ones so we do not conflate this PR with both enhancements and code cleanup.

cc @bzwei

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @gmcculloug

Copy link
Author

Choose a reason for hiding this comment

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

@gmcculloug Done.

@gmcculloug gmcculloug self-assigned this Oct 26, 2018
@miq-bot
Copy link
Member

miq-bot commented Oct 30, 2018

Checked commit fabiendupont@113b3fe with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@gmcculloug gmcculloug merged commit a0a7e52 into ManageIQ:master Oct 30, 2018
@gmcculloug gmcculloug added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 30, 2018
@ghost ghost mentioned this pull request Oct 30, 2018
Merged
simaishi pushed a commit that referenced this pull request Oct 31, 2018
@gmcculloug @simaishi
Merge pull request #263 from fdupont-redhat/bz1640816
2b84cbe 
Expose validate_conversion_hosts for TransformationPlanRequest

(cherry picked from commit a0a7e52)

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

Hammer backport details:

$ git log -1
commit 2b84cbe753cb9f8ba1febc4ec35455f7eac017a7
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Oct 30 10:39:11 2018 -0400

    Merge pull request #263 from fdupont-redhat/bz1640816
    
    Expose validate_conversion_hosts for TransformationPlanRequest
    
    (cherry picked from commit a0a7e527a936ebdaae411b06e00cba3795da6f47)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1640816

@ghost ghost mentioned this pull request Nov 13, 2018
Closed
@Fryguy Fryguy added v2v and removed transformation labels Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gmcculloug gmcculloug

Labels
blocker enhancement hammer/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JPrause @miq-bot @simaishi @gmcculloug @bzwei @Fryguy @fabiendupont