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

Do not cache the GitHub client #812

Closed
wants to merge 1 commit into from

Conversation

jalaziz
Copy link

@jalaziz jalaziz commented Mar 25, 2021

Caching the GitHub client has the unfortunate side effect of caching the
credentials too. This prevents GHRP from effectively using Github App
credentials as the returned credentials expire after an hour. By
returning a new GitHub client on demand, the credentials are fresh for
each action and generally last long enough for most, if not, all GHRP
tasks.

I'm not sure if this is the best approach, but it seems to work fine.

A seemingly better approach would be to use AuthorizationProviders available in newer versions of the Github API plugin. However, it seems this would mean that GHRP would need to depend on the Github Branch Source Plugin to use Github App credentials and its associated auth provider.

I completely understand if this is an unacceptable change, but it would be nice to support Github App authentication as it avoids needed to create a bot user account and provides fine grained permissions.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Caching the GitHub client has the unfortunate side effect of caching the
credentials too. This prevents GHRP from effectively using Github App
credentials as the returned credentials expire after an hour. By
returning a new GitHub client on demand, the credentials are fresh for
each action and generally last long enough for most, if not, all GHRP
tasks.
@jalaziz
Copy link
Author

jalaziz commented Mar 25, 2021

Turns out this approach still fails with 401s after some time in some cases.

Going to give updating to use AuthorizationProvider a shot.

@jalaziz jalaziz closed this Mar 25, 2021
@udangel-r7
Copy link

@jalaziz were you successful in using github applications for the pull request builder?

@jalaziz
Copy link
Author

jalaziz commented May 7, 2021

@jalaziz were you successful in using github applications for the pull request builder?

Unfortunately not.

I started working on support for GitHub Applications, but it would either require 1) making certain private GitHub Branch Source Plugin APIs public, 2) Porting GitHub App support from the branch source plugin to the GitHub plugin, or 3) Copying all the supporting classes into this plugin.

Other priorities took over and I haven't had time to actually start down one of those paths.

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

Successfully merging this pull request may close these issues.

2 participants