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

Fix request denial #471

Merged
merged 2 commits into from
Nov 13, 2018
Merged

Fix request denial #471

merged 2 commits into from
Nov 13, 2018

Conversation

ghost
Copy link

@ghost ghost commented Nov 13, 2018

When denying the request, the method call is wrong. This PR fixes it.

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

@ghost
Copy link
Author

ghost commented Nov 13, 2018

@miq-bot add-label transformation, bug, hammer/yes, blocker
@miq-bot add-reviewer gmcculloug

@coveralls
Copy link

coveralls commented Nov 13, 2018 •
edited
Loading

Pull Request Test Coverage Report for Build 2340

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.273%
Totals Coverage Status
Change from base Build 2333: 0.0%
Covered Lines: 2925
Relevant Lines: 3007
💛 - Coveralls

gmcculloug
gmcculloug reviewed Nov 13, 2018
View reviewed changes
...sformation/StateMachines/TransformationPlanRequestApproval.class/__methods__/deny_request.rb
@@ -2,4 +2,4 @@
message = $evm.object['reason']
$evm.log('info', "Request denied because of #{message}")
request.message = message
request.deny('admin', msg = message)
request.deny('admin', message)
Copy link
Member

Choose a reason for hiding this comment

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

@fdupont-redhat I do not see how this would do anything more than assign an unused variable msg. The result of the assignment msg = message would return the content of the message variable and passed that as the second parameter to the deny method.

While this is a reasonable cleanup I am concerned that this is not actually fixing any bug.

Copy link
Author

Choose a reason for hiding this comment

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

@gmcculloug You're completely right. Going too fast. The problem is more likely the name of the method in deny_request.yml... Fixed in my appliance in an overlay domain, but not in ManageIQ. My bad.

@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2018

Checked commits fabiendupont/manageiq-content@f3af74a~...1c6b40c 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 4e2e229 into ManageIQ:master Nov 13, 2018
@gmcculloug gmcculloug added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 13, 2018
@ghost ghost mentioned this pull request Nov 13, 2018
Closed
simaishi pushed a commit that referenced this pull request Nov 13, 2018
@gmcculloug @simaishi
Merge pull request #471 from fdupont-redhat/v2v_fix_request_denial
c4ce40a 
Fix request denial

(cherry picked from commit 4e2e229)

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

Hammer backport details:

$ git log -1
commit c4ce40ad4f060bce13b907ee32771fd01279ba6e
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Nov 13 10:25:29 2018 -0500

    Merge pull request #471 from fdupont-redhat/v2v_fix_request_denial
    
    Fix request denial
    
    (cherry picked from commit 4e2e22969181d60b5e1fd76d0db6da4f46df7313)
    
    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
Reviewers
No reviews
Assignees
No one assigned
Labels
blocker bug hammer/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@coveralls @miq-bot @simaishi @gmcculloug @fabiendupont