Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

remove the caching and use GithubAppCredentials from github source br… #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ataylor-r7
Copy link

@ataylor-r7 ataylor-r7 commented May 24, 2021

Tldr; This allows GHPRB to constantly pull updated credentials and also gives the option to use GithubAppCredentials from github source branch which handles rotating the app installation token every hour.

Why:

  1. Wanted the ability to rotate Github keys used in Jenkins
  2. GHPRB was caching in 2 ways: it was caching the credential IDs associated with the GHPRB and it was caching the token associated with each credential ID at the time a new trigger was created/updated (not when the credential itself was updated though)

Other interest we found:
jenkinsci#812
jenkinsci#813

How:

  1. Rebuild the connection with Github from GHPRB every time it interacts with the API in order to get the most updated credential ID from GHPRB config and the most updated secret associated with that ID in credentials store
  2. Use GithubAppCredentials from github source branch plugin which handles token refresh of application installation tokens (they rotate every hour)
  3. If GHPRB has a credential of instance GithubAppCredential, use Connection class from github source branch plugin vs the builder in ghprb-plugin to generate Github object

Benefits:

  1. GHPRB is no longer caching Github credentials which allows for easy/programatic update
  2. Github doesn't have an API endpoint to generate new tokens so we can rotate credentials - this allows for the use a Github App that rotates an application installation token every hour
  3. Github source branch plugin was using GithubAppCredentials but required a Jenkinsfile and the creation of an organization to do so, this keeps all the same functionality of the original GHPRB with the added option of using the GithubAppCredential class and connection from github source branch plugin

**
Right now on "mvn install" this fails with spotbugs plugin but packages fine with "mvn package"
Failure starts with:

[INFO] --- spotbugs-maven-plugin:4.2.2:check (spotbugs) @ ghprb ---
[INFO] BugInstance size is 16
[INFO] Error size is 0
[INFO] Total bugs: 16

It was also failing on 2 unit tests originally bc my changes broke what the test was expecting but can't replicate that presently, so I'll leave the tests as long as those don't show up again.

@ataylor-r7 ataylor-r7 changed the title remove the caching and use GithubAppCredentials from github source br… PD-29456 - remove the caching and use GithubAppCredentials from github source br… May 25, 2021
@cdoughty-r7 cdoughty-r7 changed the title PD-29456 - remove the caching and use GithubAppCredentials from github source br… remove the caching and use GithubAppCredentials from github source br… May 25, 2021
Copy link

@cdoughty-r7 cdoughty-r7 left a comment

Choose a reason for hiding this comment

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

I'm approving but before we try to put up a PR to merge this into the source repo, IMO we should:

  • Create an update in the changelog
  • Create additional details in the readme for usage
  • Construct a more detailed PR description with things like: why we did this change, how we did this change, what problems this change solves and any details about deploying/building (such as the errors you mentioned initially).

Copy link

@cdoughty-r7 cdoughty-r7 left a comment

Choose a reason for hiding this comment

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

looks great!

@1upD
Copy link

1upD commented Oct 12, 2021

Hope you're all doing well. I've been having some issues setting up authentication with a GitHub App with Jenkins and I think this pull request might resolve my issues. I was wondering, are you going to open a pull request with these changes back into jenkinsci/ghprb-plugin?

@cdoughty-r7
Copy link

I think the goal is to definitely push this back up to jenkinsci/ghprb-plugin however we've not been able to do so recently due to time mostly.

@cdoughty-r7
Copy link

PR opened against parent repo: jenkinsci#837

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants