Bug 2048352 - Migrate code-review bot to push to Github #3490
Draft
nahadji wants to merge 8 commits into
Draft
Conversation
Mirror the Mercurial Repository interface in a new GitRepository class so the analysis workflow can apply a Phabricator patch stack and push it to a Git remote (e.g. a GitHub "try" repository) instead of hg.mozilla.org/try. This is additive: the Mercurial path is unchanged. The repository class to use will be selected per-repository in a later change. Differences from the Mercurial implementation: - the base revision is already a Git hash, so there is no Lando git2hg lookup; - authentication uses an SSH deploy key via GIT_SSH_COMMAND. Add tests (test_git.py) covering patch application, try_task_config.json, push to a local bare repo, base-revision resolution, clean, and the diff-header normalisation. They are self-contained (local git only) and need no Mercurial environment.
Add a GitWorker mirroring MercurialWorker for Git: clean the clone, apply the Phabricator patch stack, write try_task_config.json, push to the remote try repository and return a Treeherder link. Unlike the Mercurial worker there is no treestatus wait (Git has no "try" tree); eligible push errors are retried with exponential backoff. Also make clean() equivalent to the Mercurial one. apply_build now checks out the base with a detached HEAD so the bot's patch and try_task_config commits stay throwaway drafts, and clean() refreshes from the remote (when configured) and resets back to the base. This prevents a reused cached clone from accumulating commits across builds. Add tests for the worker success path, skippable patches, general failures, retry-without-treestatus, and that a reused clone drops the previous build's commits.
Add an optional repo_type field to RepositoryConf (default "hg" so existing repository secrets keep working) and branch start_analysis on it: a repository with repo_type "git" builds a GitRepository + GitWorker using the Git cache, while everything else keeps the Mercurial Repository + MercurialWorker. Add tests: repo_type defaults to hg and accepts git, and start_analysis picks the right backend for each repo type.
The publication layer only knew the fail:mercurial mode, so a fail:git result from the GitWorker fell through to "Unsupported publication": the Phabricator build was never marked as failed and stayed in a running state forever, and Lando was never notified. Handle fail:git in publish_analysis_phabricator with a failure message mirroring the Mercurial one, and publish the existing patch-failure warning to Lando for both VCS modes.
… Git path GitRepository.get_base_identifier eagerly fell back to the default revision when the base was missing locally, so apply_build could never set missing_base_revision on the build and the "your patch has been rebased" warning was never published to Phabricator. Return the base revision as-is, mirroring the Mercurial implementation, and let apply_build record the fallback. clean() silently kept the previous build's commits when default_revision was left to its bare HEAD default without an origin remote to resolve against. Resolve the pristine base explicitly (remote-tracking branch first) and raise a clear error when none can be determined, and check it out detached instead of moving a local branch.
…ecret The deploy key used to push to Git try repositories lives in its own Taskcluster secret (field ssh_privkey), separate from the bot's runtime configuration secret whose tokens are shared with other environments. Add an optional --git-ssh-key-secret argument (GIT_SSH_KEY_SECRET in the environment) naming that secret. When set, the bot fetches it and Git repositories push with this key, falling back to the global ssh_key otherwise. The secret name stays configuration so the key's final location can be changed without a code change.
…fective test_phabricator_analysis.py imported code_review_bot.workflow at module level, which binds taskcluster.utils.stringDate inside the workflow module at collection time, before the autouse mock_taskcluster_date fixture can patch it. The frozen date then never applied to Workflow.index, breaking the date assertions of test_index.py when the whole suite runs. Import the module inside the test that needs it, like the other tests do.
…token Replace the SSH deploy key authentication with short-lived GitHub App installation tokens, as suggested by the maintainers. The bot reads GITHUB_APP_ID and GITHUB_APP_PRIVKEY from the runtime configuration secret, generates a token restricted to the try repository with simple-github (cached for the run, installation tokens are valid one hour), and pushes over HTTPS with the token injected at push time. The token is never written to disk nor to the git configuration, and read operations keep using plain urls as the repositories are public. This removes the ssh key temporary file handling and the dedicated --git-ssh-key-secret argument.
La0
requested changes
Jul 22, 2026
La0
left a comment
La0
left a comment
Collaborator
There was a problem hiding this comment.
There is a lot of code duplication for the new GitWorker & GitRepository classes, you should create a base class that shares as most code as possible between both git & hg implementations, then inherit from these base classes into specific implementations.
The ideal way to do that is to make a first PR which splits the existing Worker & Repository into a base class + specific mercurial implementation, then create another PR on top that implements the git part (what you did here).
Things like the configuration in __init__, patch normalization, the whole workflow to apply a build (albeit specific git/mercurial applications which can be further extracted), try commit build, skippable commit detection, retry detection, the overall worker run can be shared across classes.
You should end up with something like that:
flowchart TD
base_worker[BaseWorker] --> hg_worker[MercurialWorker]
base_worker[BaseWorker] --> git_worker[GitWorker]
base_repo[BaseRepository] --> hg_repo[MercurialRepository]
base_repo[BaseRepository] --> git_repo[GitRepository]
| taskcluster.secrets["ssh_key"], | ||
| args.mercurial_repository, | ||
| args.github_repository, | ||
| github_app_id=taskcluster.secrets["GITHUB_APP_ID"], |
Collaborator
There was a problem hiding this comment.
It would be nicer to create a dedicated GITHUB maaping in the conf with the following parts:
app_idapp_privkey
This would allow adding more configuration later on (as the needs for Github will certainly grow in that code base).
Also you need to document these new options there : https://github.com/mozilla/code-review/blob/master/docs/configuration.md
| "RepositoryConf", | ||
| "name, try_name, url, try_url, decision_env_prefix, ssh_user", | ||
| "name, try_name, url, try_url, decision_env_prefix, ssh_user, repo_type", | ||
| # repo_type is optional and defaults to Mercurial so existing repository |
Collaborator
There was a problem hiding this comment.
Please document that in https://github.com/mozilla/code-review/blob/master/docs/configuration.md
| ) | ||
| worker = GitWorker() | ||
| else: | ||
| repository = Repository( |
Collaborator
There was a problem hiding this comment.
You should rename Repository to MercurialRepository to avoid any confusion
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a Git push path to the code-review bot so the analysis workflow can push
patch stacks to a GitHub repository instead of hg.mozilla.org/try
(Bug 2048352).
The change is additive: the Mercurial path is untouched, and the Git backend is
selected per repository via a new optional
repo_typefield in therepositories configuration (defaults to
hg, so existing configurations keepworking unchanged).
What's included
GitRepository: applies a Phabricator patch stack as Git commits (preservingauthorship), writes the same
try_task_config.json, and force-pushes to aconfigured branch on the remote. No Lando
git2hglookup (the base isalready a Git hash); missing bases fall back to the default revision and are
reported to Phabricator like the Mercurial path does.
GitWorker: mirrorsMercurialWorker, minus the treestatus wait (no "try"tree to gate on). Eligible push errors are retried with exponential backoff.
start_analysisbased onrepo_type.fail:gitresults are reported to Phabricator and Lando.installation token (generated per run with simple-github, restricted to the
try repository, never written to disk). The bot reads GITHUB_APP_ID and
GITHUB_APP_PRIVKEY from the runtime configuration secret.
Verified
tests/test_git.py, extendedtests/test_phabricator_analysis.py);self-contained (local git repos, no Mercurial environment needed).
mozilla-releng/staging-firefoxwith a write deploy key (patch stack applied,try_task_config.jsoncommitted, branch pushed and verified, then deleted).