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

Add retirement initiator context #17951

Merged
merged 1 commit into from
Oct 17, 2018
Merged

Add retirement initiator context #17951

merged 1 commit into from
Oct 17, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Sep 6, 2018 •
edited by kbrock
Loading

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

If a system retirement goes into retry, there is an opportunity to reset the user because we're just checking the current user. This fixes that by adding a context and setting the user id, group id, and tenant id, which previously were not being set.

The default was set always originally to system. We then update it if there was a requester set. But the code to set the requester is just going off of a current user which can change.

... and ManageIQ/manageiq-schema#288 is related but not required to merge this, it's a later enhancement that'll require subsequent modification of the retirement mixin like what's happening here

related PRs:

@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 6, 2018

@miq-bot assign @gmcculloug
@miq-bot add_label bug
@miq-bot add_reviewer @tinaafitz

@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 6, 2018

@miq-bot add_label gaprindashvili/yes

@miq-bot miq-bot added the bug label Sep 6, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 6, 2018

Tina pointed out that we'll need to adjust this due to the retirement as a request because at https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/retirement_mixin.rb#L126 we need to know which context is applicable.

@d-m-u d-m-u force-pushed the fixing_system_retirement_user branch 3 times, most recently from 1d42b76 to 5e53746 Compare September 10, 2018 18:20
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 11, 2018

It's only passing tests cause there were no tests. Travis didn't run on this. It's actually failing everything. Don't be fooled.

mkanoor
mkanoor reviewed Sep 11, 2018
View reviewed changes
app/models/mixins/retirement_mixin.rb Outdated
@@ -229,6 +238,10 @@ def error_retiring?

private

def find_requester(requester_context, q_options = {})
requester_context == "system" ? q_options[:user_id] : User.current_user
Copy link
Contributor

Choose a reason for hiding this comment

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

@d-m-u This could still be nil if the q_options[:userid] is not set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, @tinaafitz, I'm not sure how to handle that

Copy link
Member

Choose a reason for hiding this comment

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

@mkanoor @d-m-u How about we remove q_options from the find_requester method, and instead use a system_requester method to to return the objects owner user object?

tinaafitz
tinaafitz reviewed Sep 11, 2018
View reviewed changes
app/models/mixins/retirement_mixin.rb Outdated
@@ -123,7 +123,8 @@ def retirement_check
end
end

self.class.make_retire_request(self.id) if retirement_due?
requester = find_requester(initiator_context(retirement_requester))
Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u Since retirement_check is called by the scheduler, the initiator_context here would always be "system".

app/models/mixins/retirement_mixin.rb Outdated
@@ -229,6 +238,10 @@ def error_retiring?

private

def find_requester(requester_context, q_options = {})
requester_context == "system" ? q_options[:user_id] : 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.

@mkanoor @d-m-u How about we remove q_options from the find_requester method, and instead use a system_requester method to to return the objects owner user object?

app/models/mixins/retirement_mixin.rb Show resolved Hide resolved
@d-m-u d-m-u force-pushed the fixing_system_retirement_user branch 4 times, most recently from 3b18455 to 4f27777 Compare September 11, 2018 18:07
@d-m-u d-m-u changed the title [WIP] Add retirement initiator context Add retirement initiator context Sep 12, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 12, 2018

@miq_bot remove_label wip

@miq-bot miq-bot removed the wip label Sep 12, 2018
@d-m-u d-m-u changed the title Add retirement initiator context [WIP] Add retirement initiator context Sep 18, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 18, 2018

@tinaafitz ping

@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 18, 2018

@miq-bot add_label wip

@miq-bot miq-bot added the wip label Sep 18, 2018
@d-m-u d-m-u force-pushed the fixing_system_retirement_user branch 3 times, most recently from 6cfc8e8 to 47fe9a0 Compare September 20, 2018 18:57
@d-m-u d-m-u force-pushed the fixing_system_retirement_user branch 6 times, most recently from 65aaea8 to d742ffe Compare October 16, 2018 21:49
@kbrock
Copy link
Member

kbrock commented Oct 16, 2018

apparently somebody gave you bad advice :(

rspec ./spec/models/service/retirement_management_spec.rb:24
rspec ./spec/models/vm/retirement_management_spec.rb:172

@d-m-u d-m-u force-pushed the fixing_system_retirement_user branch from d742ffe to a245c8b Compare October 17, 2018 12:55
@miq-bot
Copy link
Member

miq-bot commented Oct 17, 2018

Checked commit d-m-u@a245c8b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🏆

@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 17, 2018

@bdunne @tinaafitz @gmcculloug @kbrock @mkanoor -- I don't suppose a one of you wants to approve this, by any chance, pretty please?

tinaafitz
tinaafitz approved these changes Oct 17, 2018
View reviewed changes
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@d-m-u Looks good.
@mkanoor Please review.

@kbrock
Copy link
Member

kbrock commented Oct 17, 2018

I'm hoping you can get rid of this line. It has made me very sad for many moons.

@kbrock kbrock merged commit e666425 into ManageIQ:master Oct 17, 2018
@kbrock kbrock added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 17, 2018
@kbrock kbrock assigned kbrock and unassigned gmcculloug Oct 17, 2018
@d-m-u d-m-u deleted the fixing_system_retirement_user branch October 17, 2018 17:46
@d-m-u d-m-u mentioned this pull request Oct 17, 2018
Merged
simaishi pushed a commit that referenced this pull request Oct 17, 2018
@kbrock @simaishi
Merge pull request #17951 from d-m-u/fixing_system_retirement_user
9b78d52 
Add retirement initiator context

(cherry picked from commit e666425)

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

Hammer backport details:

$ git log -1
commit 9b78d522d1c1e7c062a91e8415e26611a1969a8d
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Wed Oct 17 12:26:29 2018 -0400

    Merge pull request #17951 from d-m-u/fixing_system_retirement_user
    
    Add retirement initiator context
    
    (cherry picked from commit e6664250502bfafdfdd6b3734618744ada79c9ea)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1618530

This was referenced Oct 18, 2018
Merged
Closed
Closed
@simaishi
Copy link
Contributor

simaishi commented Nov 5, 2018

Backported to Gaprindashvili via #18106

@kbrock kbrock mentioned this pull request Sep 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkanoor mkanoor mkanoor left review comments

@gmcculloug gmcculloug gmcculloug left review comments

@bdunne bdunne bdunne left review comments

@kbrock kbrock kbrock left review comments

@tinaafitz tinaafitz tinaafitz approved these changes

Assignees

@kbrock kbrock

Labels
blocker bug gaprindashvili/backported hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@d-m-u @gmcculloug @mkanoor @simaishi @kbrock @miq-bot @tinaafitz @bdunne