Git services: allow updating a single repository #12429
Conversation
Documentation build overview
Show files changed (6 files in total): 📝 6 modified | ➕ 0 added | ➖ 0 deleted
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the repository syncing mechanism for SSO organizations from syncing all user repositories to updating individual repositories. The change replaces a task that triggered bulk user repository syncs with a more targeted approach that calls update_repository on specific repositories connected to SSO organizations, excluding GitHub Apps which are updated via webhooks.
Key changes:
- Replaced the general repository sync task with a repository-specific update task
- Added
update_repositorymethods to all OAuth service providers - Updated the scheduled task configuration and corresponding tests
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| readthedocs/settings/base.py | Updated scheduled task name and function reference |
| readthedocs/oauth/tasks.py | Replaced bulk user sync logic with targeted repository updates |
| readthedocs/oauth/services/base.py | Added abstract update_repository method to base service class |
| readthedocs/oauth/services/github.py | Implemented GitHub-specific repository update with permission checking |
| readthedocs/oauth/services/githubapp.py | Implemented GitHub App repository update using existing sync logic |
| readthedocs/oauth/services/gitlab.py | Implemented GitLab repository update with permission validation |
| readthedocs/oauth/services/bitbucket.py | Implemented Bitbucket repository update with role-based access checking |
| readthedocs/rtd_tests/tests/test_oauth_tasks.py | Updated tests to verify new repository update behavior |
| readthedocs/rtd_tests/tests/test_oauth.py | Added comprehensive test coverage for all service update methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
This looks like a nice refactor in general. I wish there was a cleaner way to handle the various permissions structures of the different Git providers. It feels very likely to break if they ever change their APIs slightly, but I don't know a better way to handle it.
Definitely feels like BB is the jankiest, forcing us to iterate everything. I wonder if we should just update all the repos whenever we do anything with BB, since we're already pulling them via the API?
readthedocs/oauth/services/gitlab.py
Outdated
| def _has_access_to_repository(self, fields): | ||
| """Check if the user has access to the repository, and if they are an admin.""" | ||
| permissions = fields.get("permissions", {}) | ||
| project_access = permissions.get("project_access", {}) or {} |
There was a problem hiding this comment.
I'm a little confused on the or {} bit. Wouldn't it already return {} as a default?
There was a problem hiding this comment.
projects_access can have null as value as well, in that case the default won't work (we have tests for this).
There was a problem hiding this comment.
I should be able to just omit the default from .get and rely on the or operation
BB allows filtering on the server side, we don't actually iterate over each repository in that case readthedocs.org/readthedocs/oauth/services/bitbucket.py Lines 175 to 179 in fd8b8fc |
2 participants
When syncing permissions for SSO, we don't need to sync all repositories the user has access to, we just need to update the permissions of the repositories that are connected to projects that belong to SSO organizations. So this adds a way to just update one single repository at a time for all providers. We don't need to do this for GH app projects, since they are kept up to date via a webhook.
Since this task should be faster now, we could run it more frequently, so permissions are reflected faster on RTD.