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

Pass userid before going to automation #54

Merged
merged 1 commit into from
Jul 5, 2017
Merged

Pass userid before going to automation #54

merged 1 commit into from
Jul 5, 2017

Conversation

moolitayer
Copy link

@moolitayer moolitayer commented Jul 3, 2017 •
edited
Loading

Calculating user in raw_scan_job_create is too late since it is called from automate and always evaluates to admin at that point.

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

@moolitayer
Copy link
Author

@miq-bot add_label fine/yes, bug, smart state

@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2017

@moolitayer Cannot apply the following label because they are not recognized: smart state

@moolitayer
Copy link
Author

@enoodle @cben @simon3z please review

@simon3z
Copy link
Contributor

simon3z commented Jul 3, 2017

@moolitayer we need a BZ

@moolitayer
Copy link
Author

@moolitayer we need a BZ

Added bz

cben
cben reviewed Jul 4, 2017
View reviewed changes
app/models/manageiq/providers/kubernetes/container_manager_mixin.rb Outdated
@@ -184,7 +186,7 @@ def check_policy_prevent(policy_event, event_target, cb_method)
:class_name => self.class.to_s,
:instance_id => id,
:method_name => :check_policy_prevent_callback,
:args => [cb_method, event_target.class.name, event_target.id],
:args => [cb_method, event_target.class.name, event_target.id, User.current_user.userid],
Copy link
Contributor

@cben cben Jul 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Consider determining the userid in scan_job_create and passing it to this method.
Because the name check_policy_prevent sounds like its job is to check policy and either do or not do a task, but isn't to figure out the task.

There were some long discussions that I don't remember on this method and similar methods elsewhere... Feel free to ignore, LGTM 👍 either way.

@miq-bot
Copy link
Member

miq-bot commented Jul 4, 2017

Checked commit moolitayer@0b1a4d0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@moolitayer moolitayer changed the title pass userid before going to automation Pass userid before going to automation Jul 4, 2017
@moolitayer
Copy link
Author

@simon3z @cben please review

@cben
Copy link
Contributor

cben commented Jul 5, 2017

LGTM 👍

@simon3z simon3z merged commit e0e4b5f into ManageIQ:master Jul 5, 2017
@simaishi
Copy link

simaishi commented Aug 4, 2017

Fine backport (to manageiq repo) details:

$ git log -1
commit 48e7f137969c479d0b40ffdeac2b1a8a52817a75
Author: Federico Simoncelli <[email protected]>
Date:   Wed Jul 5 12:10:35 2017 +0200

    Merge pull request #54 from moolitayer/ssa_user
    
    Pass userid before going to automation
    (cherry picked from commit e0e4b5f427aec60e28a15cafd07345f48fcdee25)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460754

@moolitayer moolitayer added this to the Sprint 64 Ending Jul 10, 2017 milestone Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cben cben cben left review comments

@enoodle enoodle enoodle approved these changes

Assignees
No one assigned
Labels
bug fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@moolitayer @miq-bot @simon3z @cben @simaishi @enoodle