Add GitHub App service by stsewd · Pull Request #12072 · readthedocs/readthedocs.org

stsewd · 2025-03-26T17:59:25Z

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

Copilot

Pull Request Overview

This PR adds a GitHub App service to support synchronizing installations and remote repositories while maintaining backward compatibility with existing integrations. Key changes include:

Adding GitHub App–specific models, service methods, and deletion logic.
Introducing a new GitHub App webhook URL and client for integration.
Updating service registries and test cases to account for GitHub App behavior.

Reviewed Changes

Copilot reviewed 16 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
readthedocs/oauth/models.py	Added GitHubAppInstallation methods and constants support
readthedocs/oauth/urls.py	Introduced a dedicated GitHub App webhook URL
readthedocs/oauth/clients.py	Added a client for GitHub App integration
readthedocs/oauth/querysets.py	Updated querysets to exclude GitHub App related objects
readthedocs/settings/docker_compose.py	Added environment variables for GitHub App configuration
readthedocs/oauth/services/init.py	Updated the service registry to include GitHubAppService
readthedocs/settings/base.py	Added default settings for the GitHub App integration
readthedocs/settings/test.py	Added test settings including a sample RSA private key for GitHub App
readthedocs/oauth/services/base.py	Defined base methods for cloning token retrieval, updated with GitHub App behavior
readthedocs/projects/models.py	Updated GitHub project checks to consider both GitHub services
readthedocs/urls.py	Included OAuth URL patterns for handling GitHub App webhooks
readthedocs/rtd_tests/tests/test_oauth_tasks.py	Extended tests to include GitHubAppService sync calls

Files not reviewed (7)

docs/dev/install.rst: Language not supported
docs/dev/settings.rst: Language not supported
requirements/deploy.txt: Language not supported
requirements/docker.txt: Language not supported
requirements/pip.in: Language not supported
requirements/pip.txt: Language not supported
requirements/testing.txt: Language not supported

Comments suppressed due to low confidence (1)

readthedocs/oauth/services/base.py:334

Duplicate definitions of 'get_clone_token' exist in this class; merge the implementations to prevent unintended overriding and ensure a single consistent behavior.

def get_clone_token(self, project):

ericholscher

Overall this looks great -- it's a big change tho, and a little scary. Had a bunch of smaller comments, but I think it could also be useful to describe the architecture a bit more in our dev docs somewhere. It seems like there's a bunch of webhooks, and we're syncing data based on those, but it was hard for me to wrap my head around the whole flow of data and responsibility of each piece.

stsewd · 2025-04-01T17:49:57Z

I think it could also be useful to describe the architecture a bit more in our dev docs somewhere.

Hope this helps
https://github.com/readthedocs/readthedocs.org/pull/12072/files#diff-0bbc5fbaa17dc973c571176f0ac6bdc3849be5fe9b81aeb4847318df613740c6

ericholscher

I think it could also be useful to describe the architecture a bit more in our dev docs somewhere.

Hope this helps https://github.com/readthedocs/readthedocs.org/pull/12072/files#diff-0bbc5fbaa17dc973c571176f0ac6bdc3849be5fe9b81aeb4847318df613740c6

Yea, that's great.

This looks great to me. To confirm, when we merge this we mostly are still installing backend infrastructure, and there is still not a user-facing change with this? If so, I think I'm 👍 on merging it, but would also love for another set of eyes on it before we ship it.

stsewd · 2025-04-03T23:25:32Z

To confirm, when we merge this we mostly are still installing backend infrastructure, and there is still not a user-facing change with this?

yep, we can actually start testing it in our own repos by manually linking projects to a remote repository.

LecrisUT · 2025-04-08T08:28:02Z

+    Metadata (read only, so we read the repo collaborators),
+    Pull requests (read and write, so we can post a comment on PRs in the future).
+  - Organization permissions: Members (read only so we can read the organization members).
+  - Account permissions: Email addresses (read only, so allauth can fetch all verified emails).


It would be preferred if these permissions are on an opt-in basis. Many projects may want to start small, and i've seen plenty of them that are skeptical about every little permission. So instead it could be like

The minimum permissions necessary for this app are - Repository permissions: Contents (read only, so we can clone the repo contents) Additionally you may grand additional permissions depending on the features you wish to have - Repository permissions: Commit statuses (read and write, so we can create commit statuses)

It does require the app to read the granted permissions.

Hi, don't think GH allows for this, the permissions are defined by the app, installations can't choose a subset of permissions. What can happen is that the app requires more permissions and the user can accept or reject those new permissions, but all new installations will always require granting all permissions designated by the app.

And even if GH allows for this, not sure that we will support that case, as it requires considering more cases to support, and also support time, as some users don't know why a feature isn't working.

humitos

I'm pushing the review I was able to do today so @stsewd has some feedback from my side; but it's not finished yet. I will continue tomorrow with it.

humitos · 2025-04-09T15:36:11Z

+        # NOTE: do we need the email of the organization?
+        remote_org.email = gh_org.email


Probably not.

humitos

Looks good to me 👍🏼 Nice work!

Summary of comments:

there is a pattern that I don't like too much: variable = self.install.property raises an exception
there are some tests where we only check if a method was called, but we don't check changes in the db -- it seems we are missing easy checks we can perform there

humitos · 2025-04-15T08:50:37Z

+            },
+        }
+        r = self.post_webhook("pull_request", payload)
+        assert r.status_code == 200


What's the goal of this webhook? Just communicate to GitHub that we handled it but we do nothing? 🤔

We handle the pull_request event, but with we don't do anything with the "edited" action.

stsewd · 2025-04-15T17:37:24Z

dev docs are failing because intersphinx is using the data from the current latest version instead of this PR.

ref #12072 (comment)

stsewd added 5 commits March 26, 2025 12:16

Add GitHub App service

2f045d6

Merge branch 'main' into add-gh-app-service

7769ed4

Merge branch 'main' into add-gh-app-service

6045d3c

Install PyGithub

1f7d5e1

Sync organizations

3883df0

stsewd marked this pull request as ready for review March 26, 2025 18:58

stsewd requested a review from a team as a code owner March 26, 2025 18:58

stsewd requested a review from humitos March 26, 2025 18:58

auto-assign Bot assigned stsewd Mar 26, 2025

stsewd requested review from agjohnson and ericholscher March 26, 2025 19:01

Include docs as well

1e3aff6

stsewd requested a review from a team as a code owner March 26, 2025 19:03

Set as literal

2ee1b18

stsewd moved this to Needs review in 📍Roadmap Mar 26, 2025

stsewd added this to 📍Roadmap Mar 26, 2025

stsewd mentioned this pull request Mar 26, 2025

Pass extra environment variables for GitHub app readthedocs/common#259

Merged

stsewd requested a review from Copilot March 26, 2025 21:23

Copilot AI reviewed Mar 26, 2025

View reviewed changes

Comment thread readthedocs/oauth/models.py

ericholscher reviewed Mar 31, 2025

View reviewed changes

stsewd added 3 commits April 1, 2025 11:13

Updates from review

db4fbce

Fix nested list

586d133

Add dev docs for the GitHub App integration

8b95371

ericholscher reviewed Apr 3, 2025

View reviewed changes

Comment thread readthedocs/oauth/services/githubapp.py

Merge branch 'main' into add-gh-app-service

b8a8fd6

This was referenced Apr 8, 2025

Use actions/toolkit readthedocs/actions#30

Closed

Add release workflow readthedocs/actions#29

Closed

Change GH action path to always take current version readthedocs/actions#28

Closed

humitos mentioned this pull request Apr 8, 2025

Provides multiple language previews readthedocs/actions#26

Closed

LecrisUT reviewed Apr 8, 2025

View reviewed changes

humitos reviewed Apr 9, 2025

View reviewed changes

stsewd added 2 commits April 9, 2025 16:33

Updates from review

acd6f63

Group by installations on sync

0518dab

humitos approved these changes Apr 15, 2025

View reviewed changes

Updates from review

a562f75

stsewd merged commit a012077 into main Apr 15, 2025
4 of 6 checks passed

stsewd deleted the add-gh-app-service branch April 15, 2025 17:37

github-project-automation Bot moved this from Needs review to Done in 📍Roadmap Apr 15, 2025

stsewd mentioned this pull request Apr 17, 2025

Update log #12120

Merged

stsewd added a commit that referenced this pull request Apr 21, 2025

Update log (#12120)

7c8fced

ref #12072 (comment)

		# NOTE: do we need the email of the organization?
		remote_org.email = gh_org.email

Uh oh!

Conversation

stsewd commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stsewd commented Apr 1, 2025

Uh oh!

ericholscher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stsewd commented Apr 3, 2025

Uh oh!

LecrisUT Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

stsewd Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

humitos Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

humitos Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

stsewd Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stsewd commented Apr 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

stsewd commented Mar 26, 2025 •

edited

Loading