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

Raise event on new user creation #18052

Merged
merged 2 commits into from
Nov 6, 2018
Merged

Raise event on new user creation #18052

merged 2 commits into from
Nov 6, 2018

Conversation

juliancheal
Copy link
Member

@juliancheal
Copy link
Member Author

@miq-bot add_label gaprindashvili/yes

bdunne
bdunne reviewed Oct 4, 2018
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

No tests?

@juliancheal
Copy link
Member Author

No tests?

Thanks @bdunne I was thinking it was covered from the previous PR ☺️, but of course that was just for audit messages, not events. Tests incoming 👍

@juliancheal
Copy link
Member Author

juliancheal commented Oct 5, 2018 •
edited
Loading 

["MiqServer", 145], "user_created", {:event_details=>"User creation successful for User: Betty Boop with ID: [email protected]"}]

Just need to fix the test, as the event is being created.

@juliancheal juliancheal force-pushed the raise_event_new_login branch from 3a93483 to 6db2cc0 Compare October 5, 2018 13:04
@juliancheal
Copy link
Member Author

Hmm, @bdunne I'm not sure I'm testing this correct. As the event is being created, but I'm not getting the test to work correctly.

@juliancheal juliancheal force-pushed the raise_event_new_login branch from 6db2cc0 to 03eaea8 Compare October 9, 2018 13:39
bdunne
bdunne reviewed Oct 16, 2018
View reviewed changes
spec/models/authenticator/httpd_spec.rb Outdated
@@ -403,6 +403,12 @@ def authenticate
expect(-> { authenticate }).to change { User.where(:userid => '[email protected]').count }.from(0).to(1)
end

it "logs the success" do
allow($log).to receive(:info).with(/Event/)
expect($log).to receive(:info).with(/User creation successful for User$/)
Copy link
Member

Choose a reason for hiding this comment

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

Should this expectation be on $audit_log?

Copy link
Member

Choose a reason for hiding this comment

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

Also, the $ at the end of the regex shouldn't be there since that won't be the end of the message, right?

Copy link
Member

Choose a reason for hiding this comment

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

Also also, should we expect that there is an event queued?

Copy link
Member Author

Choose a reason for hiding this comment

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

We get the following in evm.log

[----] I, [2018-10-16T15:16:17.864540 #10740:3fc38f840e30]  INFO -- : MIQ(MiqQueue.put) Message id: [327],  id: [], Zone: [Zone 1], Role: [], Server: [], MiqTask id: [], Ident: [generic], Target id: [], Instance id: [], Task id: [], Command: [MiqEvent.raise_evm_event], Timeout: [600], Priority: [100], State: [ready], Deliver On: [], Data: [], Args: [["MiqServer", 315], "user_created", {:event_details=>"User creation successful for User: Bob Builderson with ID: [email protected]"}]

note "user_created", {:event_details=>"User creation successful for User: Bob Builderson with ID: [email protected]"}]

Yes I should be expecting an event queue too, I'm not sure how to do that.

@juliancheal juliancheal force-pushed the raise_event_new_login branch from 03eaea8 to 8d0bcd5 Compare October 16, 2018 14:23
@juliancheal
Copy link
Member Author

Just keeping the same as the other tests, is that correct?

@JPrause
Copy link
Member

JPrause commented Nov 1, 2018

@juliancheal can you update where this PR stands. Did it need further reviewing. This is needed for an impending deadline.

@juliancheal juliancheal force-pushed the raise_event_new_login branch 2 times, most recently from 59c7c77 to 9b5bfbc Compare November 5, 2018 19:00
@juliancheal juliancheal force-pushed the raise_event_new_login branch from 9b5bfbc to e8377ba Compare November 6, 2018 15:40
@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2018

Checked commits juliancheal/manageiq@adb680d~...e8377ba with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 2 offenses detected

spec/models/authenticator/ldap_spec.rb

@bdunne bdunne merged commit d952e08 into ManageIQ:master Nov 6, 2018
@bdunne bdunne added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 6, 2018
@bdunne bdunne assigned bdunne and unassigned gtanzillo Nov 6, 2018
simaishi pushed a commit that referenced this pull request Nov 7, 2018
@bdunne @simaishi
Merge pull request #18052 from juliancheal/raise_event_new_login
3c2d5cb 
Raise event on new user creation

(cherry picked from commit d952e08)

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

simaishi commented Nov 7, 2018

Hammer backport details:

$ git log -1
commit 3c2d5cbb3df34668ca51ca11ece0a5a18e733c85
Author: Brandon Dunne <[email protected]>
Date:   Tue Nov 6 11:05:25 2018 -0500

    Merge pull request #18052 from juliancheal/raise_event_new_login
    
    Raise event on new user creation
    
    (cherry picked from commit d952e0896e02ee754d29754e7a20a43125c965fb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1602136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne approved these changes

Assignees

@bdunne bdunne

Labels
bug core enhancement hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@juliancheal @JPrause @miq-bot @simaishi @bdunne @gtanzillo