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

Deny request if no conversion host is configured #455

Merged
merged 1 commit into from
Nov 5, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2018

This PR is the manageiq-content counterpart of ManageIQ/manageiq#18135. It checks the request and deny it if no conversion host is available. It also sets the message of the request, so that UI can display the information.

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

@miq-bot
Copy link
Member

miq-bot commented Oct 26, 2018

Checked commit fabiendupont@f9ac30a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

content/automate/ManageIQ/Transformation/StateMachines/TransformationPlanRequestApproval.class/methods/deny_request.rb

@JPrause
Copy link
Member

JPrause commented Oct 26, 2018

@miq-bot add_label blocker

@gmcculloug gmcculloug self-assigned this Oct 26, 2018
exit MIQ_ABORT
end

request.source_vms.each { |vm| request.approve_vm(vm) if request.validate_vm(vm) }
Copy link
Member

Choose a reason for hiding this comment

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

This should eventually move to the backend, but not in this PR.

unless request.validate_conversion_hosts
$evm.object['reason'] = 'No conversion host configured'
request.message = 'No conversion host configured'
exit MIQ_ABORT
Copy link
Member

Choose a reason for hiding this comment

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

@tinaafitz @mkanoor Should this remain exit or is it better to use raise?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gmcculloug It doesn't hurt to have the exit there, we solved the spec problem with the approach listed here

Which allows us to test different exit codes. So if there are specs for this change it should follow a similar pattern if the method is being tested directly outside of the DRb and Automate Engine framework.

@tinaafitz tinaafitz self-requested a review November 5, 2018 21:13
@gmcculloug gmcculloug merged commit 4d84575 into ManageIQ:master Nov 5, 2018
@gmcculloug gmcculloug added this to the Sprint 98 Ending Nov 5, 2018 milestone Nov 5, 2018
simaishi pushed a commit that referenced this pull request Nov 5, 2018
Deny request if no conversion host is configured

(cherry picked from commit 4d84575)

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

simaishi commented Nov 5, 2018

Hammer backport details:

$ git log -1
commit 82ef82a33f04bdb140115024c8467d3492401180
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Nov 5 16:16:42 2018 -0500

    Merge pull request #455 from fdupont-redhat/bz1640816
    
    Deny request if no conversion host is configured
    
    (cherry picked from commit 4d84575e1512153dee113fe1db14ebf6d9b72cc9)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1640816

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.

None yet

7 participants