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 check for existence of user for system_context #18437

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Add check for existence of user for system_context #18437

merged 1 commit into from
Feb 18, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Feb 6, 2019 •
edited
Loading

system_context retirement only checks to see if the evm_owner_id is present on the object, it doesn't check that that id is that of a valid user.

This cleans up the tests to have both cases, for when the user exists, and when it doesn't.

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

Depends on

#18443

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 6, 2019

@miq-bot add_label bug
@miq-bot assign @gmcculloug
@tinaafitz could you please review?
@bdunne could you please review too?

thanks.

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

d-m-u commented Feb 6, 2019

For when @simaishi looks at this, it needs to go back to g but I will need to open a separate PR for that, this isn't going to backport cleanly.

bdunne
bdunne reviewed Feb 6, 2019
View reviewed changes
spec/models/orchestration_stack/retirement_management_spec.rb
stack_with_owner.retirement_check
stack_with_owner.reload
expect(stack_with_owner.retirement_last_warn).not_to be_nil
expect(stack_with_owner.retirement_requester).to eq("admin")
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I would not have expected that. Is this okay for audit purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retirement_check only runs in the system_context -- in which someone scheduled a retirement for a future date. Our modus operandi at the moment for system_context is always to set the requester to admin.

@d-m-u d-m-u mentioned this pull request Feb 11, 2019
Merged
@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 11, 2019

@miq-bot add_label hammer/yes

@d-m-u d-m-u closed this Feb 11, 2019
@d-m-u d-m-u reopened this Feb 11, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 12, 2019

can i please get 👀 on this again?

@d-m-u d-m-u force-pushed the adding_user_existence_check branch from d4fd938 to 260dd12 Compare February 13, 2019 13:00
@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 13, 2019

is there any chance someone would like to look at this code again please?
🏏 🏏 🏏

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 13, 2019

@bdunne @gmcculloug please 👀

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 14, 2019

Anyone want to look at this?

@d-m-u d-m-u closed this Feb 14, 2019
@bdunne
Copy link
Member

bdunne commented Feb 14, 2019

@d-m-u Any reason that this was closed?

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 15, 2019

Sure, I gave up.

@d-m-u d-m-u reopened this Feb 15, 2019
@miq-bot
Copy link
Member

miq-bot commented Feb 15, 2019

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 18, 2019

Okay look, fine, I'm sorry I was a snark and closed it, it does still need review though

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 18, 2019

👻

@bdunne bdunne merged commit 265774d into ManageIQ:master Feb 18, 2019
@bdunne bdunne assigned bdunne and unassigned gmcculloug Feb 18, 2019
@bdunne bdunne added this to the Sprint 105 Ending Feb 18, 2019 milestone Feb 18, 2019
simaishi pushed a commit that referenced this pull request Feb 18, 2019
@bdunne @simaishi
Merge pull request #18437 from d-m-u/adding_user_existence_check
65aa7d0 
Add check for existence of user for system_context

(cherry picked from commit 265774d)

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

Hammer backport details:

$ git log -1
commit 65aa7d015bb1d6782b1ca8c5908d21980c33bc92
Author: Brandon Dunne <[email protected]>
Date:   Mon Feb 18 15:38:18 2019 -0500

    Merge pull request #18437 from d-m-u/adding_user_existence_check
    
    Add check for existence of user for system_context
    
    (cherry picked from commit 265774dd3ab68ae26f117e2cca073003cb5066d3)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1678476

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 19, 2019

sorry, i messed up, this should be hammer/no
@miq-bot add_label hammer/no

simaishi added a commit that referenced this pull request Feb 19, 2019
@simaishi
Revert "Merge pull request #18437 from d-m-u/adding_user_existence_ch…
a6ad2f3 
…eck"

This reverts commit 65aa7d0.
@simaishi
Copy link
Contributor

Reverted the backport

commit a6ad2f30607ece128a27d3302904f589e042f858
Author: Satoe Imaishi <[email protected]>
Date:   Tue Feb 19 10:09:56 2019 -0500

    Revert "Merge pull request #18437 from d-m-u/adding_user_existence_check"
    
    This reverts commit 65aa7d015bb1d6782b1ca8c5908d21980c33bc92.

@d-m-u d-m-u mentioned this pull request Feb 25, 2019
Merged
@d-m-u d-m-u deleted the adding_user_existence_check branch September 26, 2019 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdunne bdunne bdunne left review comments

@mkanoor mkanoor mkanoor approved these changes

@tinaafitz tinaafitz tinaafitz approved these changes

Assignees

@bdunne bdunne

Labels
bug hammer/no
Projects
None yet
Milestone
No milestone
Development

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