-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
There was a problem hiding this 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?
If you have this environment already setup locally, can you record a small GIF that shows the workflow to understand what is the UX? |
Yep.
Screencast.From.2025-01-28.16-17-42.mp4The old login option will be hidden in production, of course. |
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. |
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. |
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:
|
Ah, you are right here. I was only thinking about old users with a OAuth application in place already 🙃 |
- 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
- 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
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
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
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:
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. |
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
Ref #11780