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

Set user when queueing VM actions #326

Merged
merged 2 commits into from
Feb 21, 2018
Merged

Conversation

jntullo
Copy link

@jntullo jntullo commented Feb 20, 2018

When VM actions are queued, the user should be set so that it is available as the requester in automate.

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

Please review @lfu

@jntullo jntullo added the bug label Feb 20, 2018
@jntullo
Copy link
Author

jntullo commented Feb 20, 2018

The code climate issues mentioned are not something that should be addressed here

@@ -477,5 +478,18 @@ def request_console_vm(vm, protocol)
rescue => err
action_result(false, err.to_s)
end

def queue_options(method)
current_user = User.current_user
Copy link
Member

Choose a reason for hiding this comment

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

Does current user always exists for API requests?

Copy link
Member

Choose a reason for hiding this comment

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

yes, we set that based on the authentication (basic auth/auth/system).

@lfu
Copy link
Member

lfu commented Feb 20, 2018

@jntullo These changes don't cover VM retirement.

action_result(true, desc, :task_id => task_id)
rescue => err
action_result(false, err.to_s)
end

def destroy_vm(vm)
desc = "#{vm_ident(vm)} deleting"
task_id = queue_object_action(vm, desc, :method_name => "destroy")
Copy link
Member

Choose a reason for hiding this comment

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

hmm, was the :role missing here by mistake ? we're now introducing it with queue_options.

@abellotti
Copy link
Member

don't we need the same done for the instances controller actions ?

@jntullo
Copy link
Author

jntullo commented Feb 21, 2018

@lfu currently retirement is not queuing an action, but instead calling this method...does this still need the user? It does not seem like it would be used

When VM actions are queued, the user should be set so that it is available as the requester in automate.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1546375
@lfu
Copy link
Member

lfu commented Feb 21, 2018

@jntullo I made a PR ManageIQ/manageiq#17033 to fix the retirement issue.

@jntullo
Copy link
Author

jntullo commented Feb 21, 2018

@abellotti added it for instance actions as well

@miq-bot
Copy link
Member

miq-bot commented Feb 21, 2018

Checked commits jntullo/manageiq-api@8284f6d~...937ae6a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

@lfu
Copy link
Member

lfu commented Feb 21, 2018

It works good, even for retirement with these 2 PRs.

@abellotti abellotti added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 21, 2018
@abellotti abellotti self-assigned this Feb 21, 2018
@abellotti
Copy link
Member

Thanks @jntullo for fixing this. LGTM!! 👍

@abellotti abellotti merged commit c6e8b4f into ManageIQ:master Feb 21, 2018
@simaishi
Copy link
Contributor

simaishi commented Apr 9, 2018

Fine backport (to manageiq repo) details:

$ git log -1
commit c8aec62ba354b218bc75e0f6a88ba54dc19420f5
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Wed Feb 21 11:13:56 2018 -0500

    Merge pull request #326 from jntullo/bz_1546375
    
    Set user when queueing VM actions
    (cherry picked from commit c6e8b4f9507292bf98032a107d67a55613b50243)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565262

@simaishi
Copy link
Contributor

This PR can't go to Fine branch as is, and caused Travis failures... after talking with @abellotti, reverted the backport.

commit cb5816dfaa5d52288da60fc638e87d3cfd188514
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Wed Apr 11 12:07:49 2018 -0400

    Revert "Merge pull request #326 from jntullo/bz_1546375"
    
    This reverts commit c8aec62ba354b218bc75e0f6a88ba54dc19420f5.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1565262

simaishi pushed a commit that referenced this pull request Jul 26, 2018
Set user when queueing VM actions
(cherry picked from commit c6e8b4f)

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

Gaprindashvili backport details:

$ git log -1
commit 6b3e36c48702c48158b1aec8e86534c001bead26
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Wed Feb 21 11:13:56 2018 -0500

    Merge pull request #326 from jntullo/bz_1546375
    
    Set user when queueing VM actions
    (cherry picked from commit c6e8b4f9507292bf98032a107d67a55613b50243)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1565259

@bdunne
Copy link
Member

bdunne commented Aug 7, 2018

This change depends on schema changes that can not be backported to the fine branch, marking as fine/no.
@miq-bot rm_label fine/yes
@miq-bot rm_label fine/conflict
@miq-bot add_label fine/no

@miq-bot miq-bot added fine/no and removed fine/yes labels Aug 7, 2018
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.

7 participants