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

[JENKINS-37368] Improve performance in DownstreamPassCondition RunListener #105

Merged
merged 7 commits into from Sep 7, 2017

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Aug 29, 2017

Impersonate SYSTEM in DownstreamPassCondition's RunListener before calling getAllItems for faster ACL checks, and then verify the original authentication had read permission just before considering a build for promotion.

https://issues.jenkins-ci.org/browse/JENKINS-37368

@reviewbybees

@oleg-nenashev oleg-nenashev changed the title [Jenkins-37368] Improve performance in DownstreamPassCondition RunListener [JENKINS-37368] Improve performance in DownstreamPassCondition RunListener Aug 29, 2017
@ghost
Copy link

ghost commented Aug 29, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member

…ling DownstreamPassCondition's RunListener"

This reverts commit 80b4749.
@dwnusbaum
Copy link
Member Author

I realized that the test I added passes even if I don't reset the SecurityContext after impersonating SYSTEM, which made it useless. I've reverted that commit. I might have just been doing something wrong, so if there is a correct way to check that the SecurityContext is reset I can make a working test.

@jglick
Copy link
Member

jglick commented Sep 6, 2017

if there is a correct way to check that the SecurityContext is reset

You are clearly resetting it in a finally block so I think there is not much to test there.

If there is not already functional test coverage for the promotion condition as a whole, that would be useful to add, though IIRC this plugin has fairly extensive tests already; you can enable JaCoCo and check whether the edited code is covered, if you are nervous.

@dwnusbaum
Copy link
Member Author

Good point about JaCoCo, the code is covered by an existing test that checks that a build gets promoted.

@recampbell
Copy link
Member

@reviewbybees done

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

🐝

@oleg-nenashev
Copy link
Member

Merging & Releasing. Manually tested the patch, it seems to work fine

@oleg-nenashev oleg-nenashev merged commit 8fdcd82 into jenkinsci:master Sep 7, 2017
@dwnusbaum dwnusbaum deleted the JENKINS-37368 branch September 18, 2017 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants