Skip to content

Cache user lookups#64

Merged
samrocketman merged 1 commit into
jenkinsci:masterfrom
i386:cache-user-lookups
Oct 24, 2016
Merged

Cache user lookups#64
samrocketman merged 1 commit into
jenkinsci:masterfrom
i386:cache-user-lookups

Conversation

@i386
Copy link
Copy Markdown
Contributor

@i386 i386 commented Oct 23, 2016 •
edited
Loading

Requires #63 to be merged.

Introduces a cache for GHUser lookups. This should dramatically reduce the amount of API calls to Github.

PTAL @samrocketman

@i386 i386 mentioned this pull request Oct 24, 2016
Merged
@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016

#63 has been merged; we're on a roll 😄

@i386 i386 force-pushed the cache-user-lookups branch from 7db4686 to 76a0f3f Compare October 24, 2016 02:14
@i386
Copy link
Copy Markdown
Contributor Author

i386 commented Oct 24, 2016

@samrocketman awesome - rebased this change. Ready for your review.

@i386 i386 force-pushed the cache-user-lookups branch from 76a0f3f to f5aa322 Compare October 24, 2016 02:27
@i386
Copy link
Copy Markdown
Contributor Author

i386 commented Oct 24, 2016 •
edited
Loading

strange - this picked up a findbugs issue in GithubLogoutAction. Pushed a change to fix it.

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016 •
edited
Loading

@i386 I'm addressing the findbugs issue because it was due to a PR I merged #58 . See #68 for my fix.

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016 •
edited
Loading

Bah, #68 didn't resolve it. I'm not sure how to fix it. I'm thinking of reverting #58.

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016

I'm stumped. I opened #69 for more focused discussion.

@i386
Copy link
Copy Markdown
Contributor Author

i386 commented Oct 24, 2016 •
edited
Loading

@samrocketman I fixed the Findbugs problem in this PR. Jenkins.getInstance() is stupidly annotated as getInstance() almost always returns a reference.

i386
i386 commented Oct 24, 2016
View reviewed changes
Comment thread src/main/java/org/jenkinsci/plugins/GithubLogoutAction.java Outdated
Copy link
Copy Markdown
Contributor Author

@i386 i386 Oct 24, 2016

Choose a reason for hiding this comment

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

@samrocketman this gets around the findbugs issue (part 1)

Comment thread src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java Outdated
Copy link
Copy Markdown
Contributor Author

@i386 i386 Oct 24, 2016

Choose a reason for hiding this comment

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

@samrocketman part 2 of the findbugs issue

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016

Issue is fixed. See #69

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016

I like your fix better than #69. Do you mind rebasing? But instead, get rid of the changes I introduced in #69 in favor of yours?

@i386 i386 force-pushed the cache-user-lookups branch from 11faf9c to 7a9c36f Compare October 24, 2016 03:18
@i386
Copy link
Copy Markdown
Contributor Author

i386 commented Oct 24, 2016 •
edited
Loading

@samrocketman OK, Ive rebased my changes here so that it uses your fixes for #69 (I don't mind either style)

@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016

Fair enough.

@samrocketman samrocketman merged commit f332e34 into jenkinsci:master Oct 24, 2016
@samrocketman
Copy link
Copy Markdown
Member

samrocketman commented Oct 24, 2016

I built and tested it. +2 merged.

samrocketman added a commit that referenced this pull request Dec 3, 2016
@samrocketman
[JENKINS-40201] Fix cached users breaking authz
52ecd0f 
for GitHub organizations being used as a group in Jenkins.

Fixed bug introduced by #64.
@samrocketman samrocketman mentioned this pull request Dec 3, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@i386 @samrocketman