Skip to content

Switching to Github Apps for PR Builder #5710

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

Closed
saadmk11 opened this issue May 18, 2019 · 6 comments
Closed

Switching to Github Apps for PR Builder #5710

saadmk11 opened this issue May 18, 2019 · 6 comments
Labels
Needed: design decision A core team decision is required

Comments

@saadmk11
Copy link
Member

saadmk11 commented May 18, 2019

Details

This Issue will talk about if we want to migrate to Github Apps for PR Builder or not.
if we start migrating to Github Apps through PR Builder we can eventually migrate to Github Apps for all the Github Functionality on RTD. This will be a big transaction but this can be a start of that.

Why migrate to Github Apps?

GitHub Apps are the officially recommended way to integrate with GitHub because they offer many advantages over a pure OAuth-based integration
Read More in Details

Pros:

  • Installing a GitHub App grants the app access to a user or organization account's chosen repositories.
  • Installation access tokens are limited to specified repositories with the permissions chosen by the creator of the app. so its more secured.
  • *** GitHub Apps have a single webhook that receives the events they are configured to receive for every repository they have access to.
  • Fine-grained permissions target the specific information a GitHub App can access, allowing the app to be more widely used by people and organizations with security policies than OAuth Apps, which cannot be limited by permissions.
  • Prior to GitHub Apps, if the credentials of the user who set up the integration became invalidated (for instance, they left the company, and their GitHub OAuth apps credentials were revoked), the integration would break for all members of their organization. GitHub Apps, can link the integration to the organization itself, provide a more robust experience when staff changes.

Cons:

  • Need to be installed Manually by users.
  • User Needs to Give Permissions to Repositories Manually.
  • Some Changes in UX flow is needed.
  • *** GitHub Apps have a single webhook that receives the events they are configured to receive for every repository they have access to. (Might bring some Edge cases)
  • users need to install the App to get PR Builder functionality.
  • Some inconsistency might arise if a user give access to a repository that is not on RTD.
  • User must be an organization owner or have admin permissions in a repository to install a GitHub App on an organization. If a GitHub App is installed in a repository and requires organization permissions, the organization owner must approve the application.
  • The Checks API is currently available for developers to preview. The API may change without advance notice during the preview period. Preview features are not supported for production use.

User Flow

  • We can add a banner to RTD website with a button that redirects users to App Installation Page
  • From there users will select the repositories they want to give access to the App.
  • After Installation The Github App installation page will redirect the user to RTD.
  • if the user wants to add a new repository for PR Builder functionality they have to again give permission to that repository manually so that RTD can receive Event Notifications.

What do you think should we Migrate to Github Apps or keep it how it is until we get a major benefit from this migration?

The Initial Design for PR Builder without migrating can be found here #5705
referencing this on #5684

@saadmk11 saadmk11 added the Needed: design decision A core team decision is required label May 18, 2019
@stsewd
Copy link
Member

stsewd commented May 20, 2019

Is there a way to have both? Sounds like migrating to github apps the current auth, webhooks would require a lot of work, and I wouldn't like to block the PR builder project because of that. If is possible to use github apps only with the PR builder integration, I'll be +1 on that, so we can move the rest later.

@saadmk11
Copy link
Member Author

saadmk11 commented May 20, 2019

It will take a lot of work but I think we can have both of them together. But I'm worried about single webhook thing of github apps because we allow a github repository to be imported multiple times but it might be a problem as the central webhook will not be specific for a project.
Also do we want to migrate partially now? Or do we want to start a full migration process later on the line? Which will be a better approach for us?

@ericholscher
Copy link
Member

I agree with @stsewd. I'd like to move to GitHub Apps, but don't want to block our GSOC project on it. Is there some functionality we need from Apps for GSOC, or is this just a useful thing to do?

@saadmk11
Copy link
Member Author

Actually we can get by well without Github Apps for the PR Builder. We don't need it for any features in the PR builder. I was suggesting github Apps because github is standardising it on their platform. But I'm worried about somethings, do we actually want them or not.

  • there are some issues we can face because github apps use centralized webhook as we cant pin point the exact project with it. We can find the project but if there are multiple projects for the same guthub repo we might have some problems finding it.
  • we will have no control over which repositories we want to get access of. So if users give access to repos that is not in our platform then it will not work.
  • we have to send some kind of notification to all the users to install our app.
  • the process will not one button click like our current one. Users have to manually install the app and give permissions to the repo every time.
  • we have to tweak the UI to add some kind of action to redirect the user to the app install page.

@ericholscher what do you think? If this gets accepted i'll start digging more dip about this.

@ericholscher
Copy link
Member

I think we can use the current pattern for now. Migrating can be a future item.

@agjohnson
Copy link
Contributor

It's on our roadmap, under our ops repo. I'm going to close this for now as it's on our radar already. I'll point our issue here as you added a lot of great information for the migration to GH applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

No branches or pull requests

4 participants