Bug 2048352 - Migrate code-review bot to push to Github - #3490
Conversation
La0
left a comment
La0
left a comment
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"], |
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 |
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( |
There was a problem hiding this comment.
You should rename Repository to MercurialRepository to avoid any confusion
Add GitRepository and GitWorker as implementations of the VCS base classes, to apply a Phabricator patch stack and push it to a GitHub "try" repository. The base revision is already a Git hash so there is no Lando git2hg lookup, there is no treestatus wait (no "try" tree to gate on), and pushes are authenticated over HTTPS with a short-lived GitHub App installation token generated through simple-github and never written to disk. Patches are committed on a detached HEAD so a reused clone is restored to a pristine state by clean(). The tests are self-contained (local git repositories only) and need no Mercurial environment.
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 classes. The GitHub App credentials are provided through a new GITHUB mapping in the runtime configuration (app_id, app_privkey), leaving room for more GitHub settings as the needs grow.
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.
38599f0 to
8534ed6
Compare
2 participants
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).