Skip to content

Migrate GitHub OAuth App to GitHub App #11942

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
wants to merge 111 commits into from
Closed

Migrate GitHub OAuth App to GitHub App #11942

wants to merge 111 commits into from

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Jan 22, 2025

Ref #11780

@stsewd stsewd requested a review from a team as a code owner January 22, 2025 20:50
@stsewd stsewd requested a review from humitos January 22, 2025 20:50
@stsewd stsewd marked this pull request as draft January 22, 2025 20:50
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I haven't tested this locally, but it seems it could work.

I understand our "Login/Signup with GitHub" button will now point to the new GitHub App, right?

@humitos
Copy link
Member

humitos commented Jan 27, 2025

I haven't tested this locally, but it seems it could work.

If you have this environment already setup locally, can you record a small GIF that shows the workflow to understand what is the UX?

@stsewd
Copy link
Member Author

stsewd commented Jan 28, 2025

I understand our "Login/Signup with GitHub" button will now point to the new GitHub App, right?

Yep.

If you have this environment already setup locally, can you record a small GIF that shows the workflow to understand what is the UX?

Screencast.From.2025-01-28.16-17-42.mp4

The old login option will be hidden in production, of course.

@humitos
Copy link
Member

humitos commented Jan 29, 2025

The UX looks great to me! 🎉

I see we are showing both GitHub integrations in the user settings, which could be a little confusing. Maybe we can hide the old integration once the new one is approved if we want to.

@humitos
Copy link
Member

humitos commented Feb 3, 2025

It seems you are writing the code for a bigger migration that we talked for the first chunk of work. We wanted to scope this work down to be able to login with GitHub Application first. That will give us a lot of information about the workflow and confirm this is possible. Once there, we should be able to move forward with webhooks, clonning and all the other integrations we want to build on top of GitHub Application.

I'm not saying this work is wrong --we definitely need/want it-- but it will be safer to move forward using smaller steps to avoid such a big change all at once.

@stsewd
Copy link
Member Author

stsewd commented Feb 3, 2025

We wanted to scope this work down to be able to login with GitHub Application first. That will give us a lot of information about the workflow and confirm this is possible.

That was already done.

Exposing the new GH app as login option is the last step, we can't expose it first without having the integration working since new users won't be able to import projects.

My plan is:

  • Have the modeling and integrations working
  • Have a migration path for existing users
  • Expose new login option and hide the old one

@humitos
Copy link
Member

humitos commented Feb 3, 2025

we can't expose it first without having the integration working since new users won't be able to import projects.

Ah, you are right here. I was only thinking about old users with a OAuth application in place already 🙃

stsewd added a commit that referenced this pull request Mar 24, 2025
- When a PR preview is re-opened, make sure to reset its status as
active
- When closing a PR we don't need to take into consideration the commit,
we act based on the ID only.

Extracted from
#11942
stsewd added a commit that referenced this pull request Mar 25, 2025
- Webhooks can trigger builds to tags, not just branches
- Since webhooks already know the type of the version, we use it to
filter by results.
- verbose_name is the value we use to match branches or tags.

Extracted from
#11942
stsewd added a commit that referenced this pull request Mar 26, 2025
Since we have only one service with this type of integration, I didn't
abstract this as a general attribute (like service_installation or
similar), as we don't know if other services will ever bring something
similar, and if they do, we don't know how they would implement it.

Extracted from
#11942
@stsewd stsewd mentioned this pull request Mar 26, 2025
stsewd added a commit that referenced this pull request Apr 15, 2025
This is the big one :D

- After this PR is merged, we will start keeping in sync installations
and remote repositories, but they won't be exposed to users yet (we can
manually set up our projects to use a remote repo to test things out,
private repos are not supported with this PR).
- All operations from the old integration are supported, except for
cloning private repos (will open another PR for that).
- PyGitHub is an okay solution, it abstracts dealing with the JWT and
has types for everything. But it can also be inefficient in some cases
(like to interact with a commit, you need to retrieve the commit from
the API first), but I think that's fine for now, we can always fallback
to do this manually, and hopefully shouldn't be that much work, or the
other option is keep using PyGitHub and use a raw call
(requester.getJsonResponse("url")).
- Webhooks always recover nicely in case the installation wasn't
tracked, so if one operation fails for whatever reason (a bug, github is
down, our app is down), we can recover the state in the next operation.
Users can also manually hit "sync".
- The webhook is exposed from the `/webhook/githubapp/` URL, we can
change that if needed.
- We need to add the secrets and IDs on the ops repos.
- Could I have divided this into smaller PRs? Maybe, but that may
require more time removing the tests that are not needed...

Extracted from
#11942
@stsewd
Copy link
Member Author

stsewd commented Apr 15, 2025

This PR was mostly used as a playground to experiment how the GH app would integrate with our code base. The bigger changes are mostly done, the only things left are:

  • Adding the migration page
  • Adding/updating docs
  • Adding/updating UI elements to mention the GH app, as the process to import a project is a little different, and things like incoming webhooks/ssh keys aren't needed anymore.
  • Use the HTTP-based git token on .com

All that would be done in separate PRs, I'd still leave this PR open for now, but it would be closed eventually until I have moved the changes into other PRs.

stsewd added a commit that referenced this pull request Apr 15, 2025
stsewd added a commit that referenced this pull request Apr 15, 2025
@stsewd stsewd closed this Apr 15, 2025
@stsewd stsewd deleted the migrate-to-gh-apps branch April 15, 2025 20:34
stsewd added a commit that referenced this pull request Apr 21, 2025
Can be tested together with
readthedocs/ext-theme#570, there is a video
there showing the workflow.

What this does is groups all repositories the user has linked to a
project, and works over that. The actions the user takes are as follows:

- Connect its account to our new GH app, we validate that the new
account matches the account from the old integration.
- Install our app into the organization/users that the user has
repositories imported from. Users that are part of projects where they
don't have access to the repository owner/organization, can omit those,
so the proper owner can migrate those, or in the case of organizations,
they can request for an admin to approve the installation.
- Users can migrate their projects to the new GitHub app only if the app
is installed in the proper repository, and if they have admin access to
the repository.
- The migration basically consists of removing the old webhooks/ssh keys
from the repo as they are no longer needed, and after that changing the
remote repository to the one from the GH app.
- After all that is done, the next step is to revoke access to our old
OAuth app, the user must do this, as we can't do that on behalf of the
user. This step is recommended but not necessary required (we do make it
required in our migration page, so we aren't responsible for lingering
accounts).
- The final step is basically disconnecting the old GH OAuth app from
the social accounts of the user, once that is done, we no longer can
migrate the project on behalf of the user (or at least the part about
removing the ssh key and webhook), connecting the project to the new
remote repository can still be done, but users can also do that.

Extracted from
#11942
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.

3 participants