diff --git a/bot/code_review_bot/analysis.py b/bot/code_review_bot/analysis.py index e6b33427d..751fe5215 100644 --- a/bot/code_review_bot/analysis.py +++ b/bot/code_review_bot/analysis.py @@ -117,6 +117,25 @@ def publish_analysis_phabricator(payload, phabricator_api): build.target_phid, BuildState.Fail, unit=[failure] ) + elif mode == "fail:git": + extra_content = "" + if build.missing_base_revision: + extra_content = f" because the parent revision ({build.base_revision}) does not exist on the target repository. If possible, you should publish that revision" + + failure = UnitResult( + namespace="code-review", + name="git", + result=UnitResultState.Fail, + details="WARNING: The code review bot failed to apply your patch{}.\n\n```{}```".format( + extra_content, extras["message"] + ), + format="remarkup", + duration=extras.get("duration", 0), + ) + phabricator_api.update_build_target( + build.target_phid, BuildState.Fail, unit=[failure] + ) + elif mode == "test_result": result = UnitResult( namespace="code-review", @@ -192,10 +211,11 @@ def publish_analysis_lando(payload, lando_warnings): except Exception as ex: logger.error(str(ex), exc_info=True) - elif mode == "fail:mercurial": - # Send mercurial message to Lando + elif mode in ("fail:mercurial", "fail:git"): + # Send patch application failure message to Lando logger.info( - "Publishing code review hg failure.", + "Publishing code review VCS failure.", + mode=mode, revision=build.revision["id"], diff=build.diff_id, ) diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py index 3e12e4029..0ce508b6f 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -98,6 +98,7 @@ def main(): "ALLOWED_PATHS": ["*"], "task_failures_ignored": [], "ssh_key": None, + "GITHUB": {}, "user_blacklist": [], }, local_secrets=yaml.safe_load(args.configuration) @@ -124,6 +125,7 @@ def main(): taskcluster.secrets["ssh_key"], args.mercurial_repository, args.github_repository, + github=taskcluster.secrets["GITHUB"], ) # Setup statistics diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 3fa1cb239..090f52465 100644 --- a/bot/code_review_bot/config.py +++ b/bot/code_review_bot/config.py @@ -24,7 +24,10 @@ ) RepositoryConf = collections.namedtuple( "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 + # secrets keep working; set it to "git" to push to a Git remote instead. + defaults=("hg",), ) @@ -63,6 +66,9 @@ def __init__(self): # SSH Key used to push on try self.ssh_key = None + # GitHub App credentials used to push on Git try repositories + self.github = {} + # List of users that should trigger a new analysis # Indexed by their Phabricator ID self.user_blacklist = {} @@ -80,6 +86,7 @@ def setup( ssh_key=None, mercurial_cache=None, git_cache=None, + github=None, ): # Detect source from env if "TRY_TASK_ID" in os.environ and "TRY_TASK_GROUP_ID" in os.environ: @@ -123,13 +130,18 @@ def build_conf(nb, repo): assert isinstance( repo, dict ), "Repository configuration #{nb+1} is not a dict" - data = [] + data = {} for key in RepositoryConf._fields: - assert ( - key in repo - ), f"Missing key {key} in repository configuration #{nb+1}" - data.append(repo[key]) - return RepositoryConf._make(data) + if key in repo: + data[key] = repo[key] + elif key in RepositoryConf._field_defaults: + # Optional field, fall back to its default (e.g. repo_type) + data[key] = RepositoryConf._field_defaults[key] + else: + raise AssertionError( + f"Missing key {key} in repository configuration #{nb+1}" + ) + return RepositoryConf(**data) self.repositories = [build_conf(i, repo) for i, repo in enumerate(repositories)] assert self.repositories, "No repositories available" @@ -161,6 +173,8 @@ def build_conf(nb, repo): # Fallback to mercurial cache to ease migration on production systems self.git_cache = self.mercurial_cache + self.github = github or {} + def load_user_blacklist(self, usernames, phabricator_api): """ Load all black listed users from Phabricator API diff --git a/bot/code_review_bot/git.py b/bot/code_review_bot/git.py index b5378c775..54b535b08 100644 --- a/bot/code_review_bot/git.py +++ b/bot/code_review_bot/git.py @@ -1,10 +1,35 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import asyncio +import os +import re +import tempfile from urllib.parse import urlparse import structlog from git import Repo +from git.exc import GitCommandError +from libmozdata.phabricator import PhabricatorPatch +from simple_github import AppAuth, AppInstallationAuth + +from code_review_bot.vcs import BaseRepository, BaseWorker logger = structlog.getLogger(__name__) +# Default author for commits without explicit Phabricator author data, and the +# committer for all bot-created commits. Matches the Mercurial worker. +DEFAULT_AUTHOR_NAME = "code review bot" +DEFAULT_AUTHOR_EMAIL = "release-mgmt-analysis@mozilla.com" + +# Matches a trailing "Weekday Mon DD HH:MM:SS YYYY +ZZZZ" timestamp that some +# Phabricator/Mercurial raw diffs append to the ---/+++ header lines. `git apply` +# would treat it as part of the filename, so it is stripped before applying. +DIFF_HEADER_TIMESTAMP = re.compile( + r"[ \t]+\w{3}\s+\w{3}\s+\d{1,2}\s+\d{2}:\d{2}:\d{2}\s+\d{4}\s+[+-]\d{4}\s*$" +) + def build_repo_slug(repo_url): """ @@ -72,3 +97,263 @@ def git_clone(base_repository, head_repository, revision, destination): repo.head.reference = repo.commit(revision) return repo + + +class GitRepository(BaseRepository): + """ + A Git repository with credentials to push a patch stack to a remote + (e.g. a GitHub "try" repository). + + Notable differences from the Mercurial implementation: + - the base revision is already a Git hash, so there is no Lando ``git2hg`` lookup; + - pushes are authenticated over HTTPS with a short-lived GitHub App + installation token generated at push time. + """ + + DEFAULT_REVISION = "HEAD" + + def __init__(self, config, cache_root): + super().__init__(config, cache_root) + + # Branch pushed to the remote try repository + self.head_branch = config.get("head_branch", "code-review") + + # GitHub App credentials used to generate short-lived push tokens + self.github_app_id = config.get("github_app_id") + self.github_app_privkey = config.get("github_app_privkey") + self._github_token = None + + @property + def repo(self): + """Lazily open the local Git repository.""" + if self._repo is None: + logger.info(f"Git open {self.dir}") + self._repo = Repo(self.dir) + return self._repo + + def github_token(self): + """Short-lived GitHub App installation token for the try repository. + + Generated on first use and cached for the run (a bot run is well within + the one hour validity of installation tokens). + """ + if self._github_token is None: + assert ( + self.github_app_id and self.github_app_privkey + ), "Missing GitHub App credentials" + self._github_token = asyncio.run(self._generate_github_token()) + return self._github_token + + async def _generate_github_token(self): + parts = urlparse(self.try_url) + assert ( + parts.netloc == "github.com" + ), "GitHub App tokens only support github.com repositories" + path = parts.path.strip("/").removesuffix(".git") + owner, _, repo = path.partition("/") + auth = AppInstallationAuth( + AppAuth(self.github_app_id, self.github_app_privkey), + owner, + repositories=[repo], + ) + try: + return await auth.get_token() + finally: + await auth.close() + + def authenticated_url(self, url): + """Inject an installation token in an HTTPS GitHub url. + + Other urls (e.g. local paths in the test suite) are returned unchanged. + """ + parts = urlparse(url) + if parts.scheme not in ("http", "https"): + return url + return f"{parts.scheme}://git:{self.github_token()}@{parts.netloc}{parts.path}" + + def clone(self): + # Read operations use the plain url: the repositories are public, only + # pushes need authentication + logger.info("Checking out git repository", repo=self.url, dir=self.dir) + if os.path.isdir(os.path.join(self.dir, ".git")): + self._repo = Repo(self.dir) + self.repo.remotes.origin.fetch() + else: + self._repo = Repo.clone_from(self.url, self.dir) + logger.info("Full checkout finished") + + def has_revision(self, revision): + """Check whether a revision exists in the local Git repository.""" + if not revision: + return False + try: + self.repo.git.cat_file("-e", f"{revision}^{{commit}}") + return True + except GitCommandError: + return False + + def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: + """Return the base identifier to apply patches against. + + Unlike Mercurial, the base revision is already a Git hash, so there is + no Lando ``git2hg`` conversion. A base revision missing locally is + handled by ``apply_build``, which records it on the build and falls + back to the default revision. + """ + if self.use_latest_revision: + return self.default_revision + return needed_stack[0].base_revision + + def checkout_base(self, base): + """Move the working tree to the base revision. + + HEAD is detached so the patches committed on top stay throwaway drafts + and never advance a branch; clean() then discards them by returning to + the base. + """ + logger.info(f"Updating repo to revision {base}") + self.repo.git.checkout(base, force=True, detach=True) + + @staticmethod + def get_author(commit): + """Build a ``(name, email)`` tuple from Phabricator commit data.""" + author = commit.get("author") if commit else None + if author is None: + return DEFAULT_AUTHOR_NAME, DEFAULT_AUTHOR_EMAIL + if author.get("name") and author.get("email"): + return author["name"], author["email"] + # Fall back to parsing the raw "Name " representation + raw = author.get("raw", "") or "" + match = re.match(r"^(?P.*?)\s*<(?P.*)>\s*$", raw) + if match: + return match.group("name"), match.group("email") + return (raw or DEFAULT_AUTHOR_NAME), DEFAULT_AUTHOR_EMAIL + + @staticmethod + def normalize_patch(patch: str) -> str: + """Strip trailing timestamps from ---/+++ header lines. + + Some Phabricator/Mercurial raw diffs append a "Weekday Mon DD ..." timestamp + to the header filenames; ``git apply`` would treat it as part of the filename. + """ + lines = [] + for line in patch.splitlines(): + if line.startswith("--- ") or line.startswith("+++ "): + line = DIFF_HEADER_TIMESTAMP.sub("", line) + lines.append(line) + return "\n".join(lines) + "\n" + + def apply_patch(self, patch, message, commit): + """Apply a single unified diff to the index and commit it.""" + name, email = self.get_author(commit) + + with tempfile.NamedTemporaryFile( + mode="w", suffix=".diff", delete=False + ) as patch_file: + patch_file.write(self.normalize_patch(patch.patch)) + patch_path = patch_file.name + + try: + self.repo.git.apply("--index", patch_path) + env = { + "GIT_AUTHOR_NAME": name, + "GIT_AUTHOR_EMAIL": email, + "GIT_COMMITTER_NAME": DEFAULT_AUTHOR_NAME, + "GIT_COMMITTER_EMAIL": DEFAULT_AUTHOR_EMAIL, + } + with self.repo.git.custom_environment(**env): + self.repo.git.commit("--no-verify", "-m", message) + finally: + os.unlink(patch_path) + + def commit_try_task_config(self, path, message): + """Commit the try_task_config.json file as the bot""" + self.repo.git.add(path) + env = { + "GIT_AUTHOR_NAME": DEFAULT_AUTHOR_NAME, + "GIT_AUTHOR_EMAIL": DEFAULT_AUTHOR_EMAIL, + "GIT_COMMITTER_NAME": DEFAULT_AUTHOR_NAME, + "GIT_COMMITTER_EMAIL": DEFAULT_AUTHOR_EMAIL, + } + with self.repo.git.custom_environment(**env): + self.repo.git.commit("--no-verify", "-m", message) + + def push_to_try(self): + """Push the current HEAD to the remote try repository.""" + head = self.repo.head.commit + logger.info("Pushing patches to try", rev=head.hexsha, branch=self.head_branch) + self.repo.git.push( + self.authenticated_url(self.try_url), + f"HEAD:refs/heads/{self.head_branch}", + force=True, + ) + return head + + def revision_id(self, tip): + """Extract the revision identifier from a push_to_try result""" + return tip.hexsha + + def clean(self): + """Reset the local checkout to a pristine state. + + Mirrors the Mercurial ``clean()`` (revert + strip outgoing drafts + + pull): a reused clone can hold the patch commits and ``try_task_config`` + commit from a previous build, so we discard local changes, refresh from + the remote and return to the base revision. + """ + logger.info("Cleaning git checkout") + + # Discard uncommitted changes and untracked/ignored files + self.repo.git.reset("--hard") + self.repo.git.clean("-fxd") + + # Refresh from the remote when one is configured (mirrors hg pull) + if any(remote.name == "origin" for remote in self.repo.remotes): + self.repo.remotes.origin.fetch() + + # Return to the pristine base, dropping any previously applied commits. + # Prefer the remote-tracking base so we also pick up upstream updates. + upstream = f"origin/{self.default_revision}" + if self.has_revision(upstream): + target = upstream + elif self.default_revision != "HEAD": + target = self.default_revision + else: + # A bare HEAD cannot identify a pristine base once patches have + # been committed on top of it + raise Exception( + "Cannot determine the base to reset to: configure default_revision " + "or make sure the repository has an origin remote" + ) + self.repo.git.checkout(target, force=True, detach=True) + + +class GitWorker(BaseWorker): + """ + Git worker maintaining several local clones. + + Mirrors the Mercurial worker, without the treestatus wait: Git has no + "try" tree to gate on, so failed pushes are simply retried with backoff. + """ + + VCS_ERROR = GitCommandError + VCS_NAME = "Git" + FAILURE_MODE = "fail:git" + REPOSITORY_CLASS = GitRepository + + ELIGIBLE_RETRY_ERRORS = [ + error.lower() + for error in [ + "could not read from remote repository", + "connection closed by remote host", + "connection timed out", + "early eof", + "rpc failed", + "the remote end hung up unexpectedly", + "ssh_exchange_identification", + ] + ] + + def format_error(self, error): + """Extract a readable error log from a Git exception""" + return error.stderr or str(error) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index bcdc25537..3bd9540a1 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -18,7 +18,7 @@ ) from code_review_bot.backend import BackendAPI from code_review_bot.config import settings -from code_review_bot.git import git_clone +from code_review_bot.git import GitRepository, GitWorker, git_clone from code_review_bot.mercurial import ( MercurialRepository, MercurialWorker, @@ -276,9 +276,11 @@ def start_analysis(self, revision): "One of Mercurial cache or github cache must be configured to start analysis" ) - # Cannot run without ssh key - if not settings.ssh_key: - raise Exception("SSH Key must be configured to start analysis") + # Cannot run without a push credential + if not settings.ssh_key and not settings.github.get("app_privkey"): + raise Exception( + "An SSH key or GitHub App must be configured to start analysis" + ) # Set the Phabricator build as running self.update_status(revision, state=BuildState.Work) @@ -299,21 +301,39 @@ def start_analysis(self, revision): api_key=self.phabricator.api_key, ) - # Initialize mercurial repository - repository = MercurialRepository( - config={ - "name": revision.base_repository_conf.name, - "try_name": revision.base_repository_conf.try_name, - "url": revision.base_repository_conf.url, - "try_url": revision.base_repository_conf.try_url, - # Setup ssh identity - "ssh_user": revision.base_repository_conf.ssh_user, - "ssh_key": settings.ssh_key, - # Force usage of robustcheckout - "checkout": "robust", - }, - cache_root=settings.mercurial_cache, - ) + # Initialize the repository and worker for the configured backend. + # Git is selected per-repository via repo_type; Mercurial is the default. + base_conf = revision.base_repository_conf + if base_conf.repo_type == "git": + repository = GitRepository( + config={ + "name": base_conf.name, + "try_name": base_conf.try_name, + "url": base_conf.url, + "try_url": base_conf.try_url, + # GitHub App credentials to generate short-lived push tokens + "github_app_id": settings.github.get("app_id"), + "github_app_privkey": settings.github.get("app_privkey"), + }, + cache_root=settings.git_cache, + ) + worker = GitWorker() + else: + repository = MercurialRepository( + config={ + "name": base_conf.name, + "try_name": base_conf.try_name, + "url": base_conf.url, + "try_url": base_conf.try_url, + # Setup ssh identity + "ssh_user": base_conf.ssh_user, + "ssh_key": settings.ssh_key, + # Force usage of robustcheckout + "checkout": "robust", + }, + cache_root=settings.mercurial_cache, + ) + worker = MercurialWorker() # Try to update the state 5 consecutive time for i in range(5): @@ -344,7 +364,6 @@ def start_analysis(self, revision): repository.clone() # Apply the stack of patches and push to try - worker = MercurialWorker() output = worker.run(repository, build) # Update index when the patch has been pushed to try diff --git a/bot/requirements.txt b/bot/requirements.txt index 0145a7308..80ff046b1 100644 --- a/bot/requirements.txt +++ b/bot/requirements.txt @@ -8,5 +8,6 @@ pyyaml==6.0.3 rs_parsepatch==0.4.6 sentry-sdk==2.66.1 setuptools==83.0.0 +simple-github==3.1.0 structlog==26.1.0 taskcluster==103.0.1 diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index da6d97580..24e80dcc1 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -1187,6 +1187,62 @@ def mock_nss(tmpdir): return repo +def build_git_repository(tmpdir, name): + """ + Mock a local git repo with a single base commit (no remote access). + Mirror of build_repository() for the Mercurial tests. + """ + from git import Actor, Repo + + repo_dir = str(tmpdir.mkdir(name).realpath()) + repo = Repo.init(repo_dir) + + # Set a committer identity and disable signing for the fixture + with repo.config_writer() as cw: + cw.set_value("user", "name", "test") + cw.set_value("user", "email", "test") + cw.set_value("commit", "gpgsign", "false") + + # Commit a Readme as the base revision + readme = os.path.join(repo_dir, "README.md") + with open(readme, "w") as f: + f.write("Hello World") + repo.index.add(["README.md"]) + actor = Actor("test", "test") + repo.index.commit("Readme", author=actor, committer=actor) + + return repo + + +@pytest.fixture +def mock_mc_git(tmpdir): + """ + Mock a Mozilla Central repository backed by Git + """ + from git import Repo + + from code_review_bot.git import GitRepository + + repo = build_git_repository(tmpdir, "mozilla-central") + + # A local bare repo acts as the remote "try" target (no network access) + try_dir = str(tmpdir.mkdir("try.git").realpath()) + Repo.init(try_dir, bare=True) + + config = { + "name": "mozilla-central", + "url": "https://github.com/mozilla/test", + "try_url": try_dir, + "try_name": "try", + "default_revision": repo.active_branch.name, + "head_branch": "code-review", + } + git_repo = GitRepository(config, str(tmpdir.realpath())) + git_repo._repo = repo + git_repo.clone = MagicMock(side_effect=lambda: True) + return git_repo + + @pytest.fixture @contextmanager def PhabricatorMock(): diff --git a/bot/tests/test_git.py b/bot/tests/test_git.py new file mode 100644 index 000000000..fb5e08dc2 --- /dev/null +++ b/bot/tests/test_git.py @@ -0,0 +1,386 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +import json +import os.path +from unittest.mock import MagicMock + +import pytest +from conftest import MockBuild +from git.exc import GitCommandError + +from code_review_bot.git import GitRepository, GitWorker +from code_review_bot.vcs import MAX_PUSH_RETRIES, BaseWorker + +# A diff whose base revision exists neither in Git nor Mercurial: patches will be +# applied on the repository's default revision (mirrors the Mercurial tests). +DIFF = { + "phid": "PHID-DIFF-test123", + "revisionPHID": "PHID-DREV-deadbeef", + "id": 1234, + "baseRevision": "abcdef123456", +} + + +def make_build(phabricator_mock): + """Build a MockBuild with its patch stack loaded from the Phabricator mock.""" + build = MockBuild(1234, "PHID-REPO-mc", 5678, "PHID-HMBT-deadbeef", dict(DIFF)) + with phabricator_mock as phab: + phab.load_patches_stack(build) + return build + + +def test_normalize_patch(): + """Trailing Mercurial-style timestamps are stripped from ---/+++ headers.""" + patch = ( + "diff -r 000000000000 test.txt\n" + "--- /dev/null Thu Jan 01 00:00:00 1970 +0000\n" + "+++ b/test.txt Tue Feb 05 17:23:40 2019 +0100\n" + "@@ -0,0 +1,1 @@\n" + "+First Line\n" + ) + normalized = GitRepository.normalize_patch(patch) + assert "--- /dev/null\n" in normalized + assert "+++ b/test.txt\n" in normalized + assert "1970" not in normalized and "2019" not in normalized + + # Git-style headers (no timestamp) are left untouched + git_patch = "--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b\n" + assert GitRepository.normalize_patch(git_patch) == git_patch + + +def test_has_revision(mock_mc_git): + head = mock_mc_git.repo.head.commit.hexsha + assert mock_mc_git.has_revision(head) is True + assert mock_mc_git.has_revision(head[:12]) is True + assert mock_mc_git.has_revision("deadbeef" * 5) is False + assert mock_mc_git.has_revision("") is False + assert mock_mc_git.has_revision(None) is False + + +def test_get_base_identifier_no_git2hg(mock_mc_git): + """The git base is used directly: no Lando git2hg lookup.""" + head = mock_mc_git.repo.head.commit.hexsha + + present = MagicMock(base_revision=head) + assert mock_mc_git.get_base_identifier([present]) == head + + # An unknown base is returned as-is: apply_build detects it is missing, + # records it on the build and falls back to the default revision + absent = MagicMock(base_revision="abcdef123456") + assert mock_mc_git.get_base_identifier([absent]) == "abcdef123456" + + +def test_apply_patches(PhabricatorMock, mock_mc_git): + """Apply a Phabricator stack as Git commits onto the default revision.""" + build = make_build(PhabricatorMock) + + target = os.path.join(mock_mc_git.dir, "test.txt") + assert not os.path.exists(target) + + mock_mc_git.apply_build(build) + + # The patched file now has the expected content + assert os.path.exists(target) + assert open(target).read() == "First Line\nSecond Line\n" + + # The unknown base revision is recorded on the build with the fallback used + assert build.missing_base_revision is True + assert build.base_revision == build.stack[0].base_revision + assert build.actual_base_revision == mock_mc_git.default_revision + + # Commits (newest first): the two patches on top of the base Readme + commits = list(mock_mc_git.repo.iter_commits()) + assert [c.message.strip() for c in commits] == [ + "Bug XXX - A second commit message\nDifferential Diff: PHID-DIFF-test123", + "Bug XXX - A first commit message\nDifferential Diff: PHID-DIFF-xxxx", + "Readme", + ] + assert [f"{c.author.name} <{c.author.email}>" for c in commits] == [ + "John Doe ", + "randomUsername ", + "test ", + ] + + +def test_add_try_commit(PhabricatorMock, mock_mc_git): + """try_task_config.json is written and committed by the bot author.""" + build = make_build(PhabricatorMock) + mock_mc_git.apply_build(build) + mock_mc_git.add_try_commit(build) + + config_path = os.path.join(mock_mc_git.dir, "try_task_config.json") + assert os.path.exists(config_path) + assert json.load(open(config_path)) == { + "version": 2, + "parameters": { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + "phabricator_diff": "PHID-HMBT-deadbeef", + }, + } + + head = next(mock_mc_git.repo.iter_commits()) + assert ( + head.message.strip() + == "try_task_config for code-review\nDifferential Diff: PHID-DIFF-test123" + ) + assert ( + f"{head.author.name} <{head.author.email}>" + == "code review bot " + ) + + +def test_push_to_try(PhabricatorMock, mock_mc_git): + """push_to_try pushes the prepared HEAD to the configured branch/remote.""" + build = make_build(PhabricatorMock) + mock_mc_git.apply_build(build) + mock_mc_git.add_try_commit(build) + + pushed = mock_mc_git.push_to_try() + + assert pushed == mock_mc_git.repo.head.commit + + # The remote try repo received the configured branch at the pushed commit + from git import Repo + + remote = Repo(mock_mc_git.try_url) + assert remote.refs["code-review"].commit.hexsha == pushed.hexsha + + +def test_clean(mock_mc_git): + """clean() resets tracked changes and removes untracked files.""" + untracked = os.path.join(mock_mc_git.dir, "untracked.txt") + with open(untracked, "w") as f: + f.write("dirty") + readme = os.path.join(mock_mc_git.dir, "README.md") + with open(readme, "w") as f: + f.write("changed") + assert mock_mc_git.repo.is_dirty(untracked_files=True) + + mock_mc_git.clean() + + assert not mock_mc_git.repo.is_dirty(untracked_files=True) + assert not os.path.exists(untracked) + assert open(readme).read() == "Hello World" + + +def test_clean_drops_previous_build(PhabricatorMock, mock_mc_git): + """A reused clone does not accumulate a previous build's commits.""" + branch = mock_mc_git.default_revision + base = mock_mc_git.repo.commit(branch).hexsha + assert mock_mc_git.repo.git.rev_list("--count", branch).strip() == "1" + + # First build: apply the stack and the try_task_config commit + build = make_build(PhabricatorMock) + mock_mc_git.apply_build(build) + mock_mc_git.add_try_commit(build) + + # Those commits live on a detached HEAD; the branch has not moved + assert mock_mc_git.repo.head.is_detached + assert mock_mc_git.repo.commit(branch).hexsha == base + assert os.path.exists(os.path.join(mock_mc_git.dir, "test.txt")) + + # Cleaning returns to the pristine base, dropping the build's commits + mock_mc_git.clean() + + assert mock_mc_git.repo.head.commit.hexsha == base + assert mock_mc_git.repo.git.rev_list("--count", branch).strip() == "1" + assert not os.path.exists(os.path.join(mock_mc_git.dir, "test.txt")) + assert not os.path.exists(os.path.join(mock_mc_git.dir, "try_task_config.json")) + + +def test_clean_requires_pristine_base(tmpdir): + """Without a configured default_revision nor an origin remote, clean() + fails loudly instead of silently keeping the previous build's commits.""" + from conftest import build_git_repository + + repo = build_git_repository(tmpdir, "no-default") + config = { + "name": "no-default", + "url": "https://github.com/mozilla/test", + "try_url": str(tmpdir.mkdir("no-default-try.git").realpath()), + } + git_repo = GitRepository(config, str(tmpdir.realpath())) + git_repo._repo = repo + + with pytest.raises(Exception, match="configure default_revision"): + git_repo.clean() + + +def test_clean_picks_up_remote_updates(tmpdir, mock_mc_git): + """clean() resets onto the remote-tracking base, so upstream commits + landed since the last build are picked up (mirrors hg pull).""" + from git import Actor, Repo + + # Clone the repo to act as its origin, and move it one commit ahead + origin_dir = str(tmpdir.mkdir("origin-repo").realpath()) + origin = mock_mc_git.repo.clone(origin_dir) + with origin.config_writer() as cw: + cw.set_value("user", "name", "test") + cw.set_value("user", "email", "test") + cw.set_value("commit", "gpgsign", "false") + with open(os.path.join(origin_dir, "update.txt"), "w") as f: + f.write("upstream update") + origin.index.add(["update.txt"]) + actor = Actor("test", "test") + upstream_tip = origin.index.commit("upstream update", author=actor, committer=actor) + + mock_mc_git.repo.create_remote("origin", origin_dir) + mock_mc_git.clean() + + assert mock_mc_git.repo.head.commit.hexsha == upstream_tip.hexsha + assert os.path.exists(os.path.join(mock_mc_git.dir, "update.txt")) + # The remote in the local clone is untouched by cleanup + assert Repo(origin_dir).head.commit.hexsha == upstream_tip.hexsha + + +def test_worker_failure_git(PhabricatorMock, mock_mc_git): + """A non-retryable Git error yields a fail:git result with the error log.""" + build = make_build(PhabricatorMock) + error = GitCommandError("git apply", 128, b"fatal: corrupt patch at line 3") + mock_mc_git.apply_build = MagicMock(side_effect=error) + + worker = GitWorker() + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:git" + assert out_build is build + assert "corrupt patch" in details["message"] + + +def test_github_token(monkeypatch, tmpdir): + """An installation token is generated from the App credentials, restricted + to the try repository, and cached for the run.""" + from conftest import build_git_repository + + repo = build_git_repository(tmpdir, "app-repo") + config = { + "name": "app-repo", + "url": "https://github.com/mozilla-releng/staging-firefox", + "try_url": "https://github.com/mozilla-releng/staging-firefox.git", + "github_app_id": 12345, + "github_app_privkey": "AppPrivateKey", + } + git_repo = GitRepository(config, str(tmpdir.realpath())) + git_repo._repo = repo + + calls = [] + + class FakeInstallationAuth: + def __init__(self, app_auth, owner, repositories): + calls.append((app_auth, owner, repositories)) + + async def get_token(self): + return "generated-token" + + async def close(self): + pass + + monkeypatch.setattr( + "code_review_bot.git.AppAuth", lambda app_id, privkey: (app_id, privkey) + ) + monkeypatch.setattr("code_review_bot.git.AppInstallationAuth", FakeInstallationAuth) + + assert git_repo.github_token() == "generated-token" + assert calls == [((12345, "AppPrivateKey"), "mozilla-releng", ["staging-firefox"])] + + # Cached: no second generation + assert git_repo.github_token() == "generated-token" + assert len(calls) == 1 + + # The token is injected in https urls only + assert ( + git_repo.authenticated_url( + "https://github.com/mozilla-releng/staging-firefox.git" + ) + == "https://git:generated-token@github.com/mozilla-releng/staging-firefox.git" + ) + + +def test_authenticated_url_local_paths(mock_mc_git): + """Local paths (as used by the test remotes) are never authenticated.""" + assert mock_mc_git.authenticated_url(mock_mc_git.try_url) == mock_mc_git.try_url + + +def test_worker_run_success(PhabricatorMock, mock_mc_git): + """Full success path: apply, configure try, push, return treeherder link.""" + build = make_build(PhabricatorMock) + + worker = GitWorker() + result = worker.run(mock_mc_git, build) + + tip = mock_mc_git.repo.head.commit + assert result == ( + "success", + build, + { + "revision": tip.hexsha, + "treeherder_url": ( + "https://treeherder.mozilla.org/#/jobs?repo=try&revision=" + f"{tip.hexsha}" + ), + }, + ) + + # The remote try repo received the configured branch at the pushed commit + from git import Repo + + remote = Repo(mock_mc_git.try_url) + assert remote.refs["code-review"].commit.hexsha == tip.hexsha + + +def test_worker_skippable(PhabricatorMock, mock_mc_git): + """A patch touching only skippable files is not pushed to try.""" + build = make_build(PhabricatorMock) + + worker = GitWorker(skippable_files=["test.txt"]) + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:ineligible" + assert out_build is build + assert "skippable" in details["message"] + # Nothing was pushed + from git import Repo + + assert "code-review" not in Repo(mock_mc_git.try_url).refs + + +def test_worker_failure_general(PhabricatorMock, mock_mc_git): + """A non-Git error while applying yields a fail:general result.""" + build = make_build(PhabricatorMock) + mock_mc_git.apply_build = MagicMock(side_effect=Exception("boom")) + + worker = GitWorker() + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:general" + assert details["message"] == "boom" + + +def test_worker_retry_no_treestatus(PhabricatorMock, mock_mc_git, monkeypatch): + """Eligible push errors are retried (no treestatus wait) up to the max.""" + build = make_build(PhabricatorMock) + + # Isolate the worker's retry logic from the repo mechanics + mock_mc_git.clean = MagicMock() + mock_mc_git.apply_build = MagicMock() + mock_mc_git.add_try_commit = MagicMock() + error = GitCommandError( + "git push", 128, b"fatal: Could not read from remote repository" + ) + mock_mc_git.push_to_try = MagicMock(side_effect=error) + + # Don't actually wait through the exponential backoff + monkeypatch.setattr("code_review_bot.vcs.time.sleep", lambda *a, **k: None) + + worker = GitWorker() + + # The Git worker keeps the default no-op hook: no treestatus gate + assert GitWorker.wait_try_available is BaseWorker.wait_try_available + + mode, out_build, details = worker.run(mock_mc_git, build) + + assert mode == "fail:git" + # Initial attempt + one per retry + assert mock_mc_git.push_to_try.call_count == MAX_PUSH_RETRIES + 1 diff --git a/bot/tests/test_phabricator_analysis.py b/bot/tests/test_phabricator_analysis.py index 5e64eec6c..24c005447 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -7,10 +7,13 @@ from unittest import mock import pytest -from libmozdata.phabricator import ConduitError +from libmozdata.phabricator import BuildState, ConduitError from code_review_bot import mercurial from code_review_bot.analysis import ( + LANDO_FAILURE_HG_MESSAGE, + PhabricatorRevisionBuild, + publish_analysis_lando, publish_analysis_phabricator, ) from code_review_bot.config import RepositoryConf @@ -270,3 +273,124 @@ def test_publish_analysis_phabricator_reraises_other_conduit_errors(): payload = ("success", build, {"treeherder_url": "https://treeherder.mozilla.org/"}) with pytest.raises(ConduitError): publish_analysis_phabricator(payload, phabricator_api) + + +def test_repository_conf_repo_type(): + """repo_type is optional, defaults to hg, and can be set to git (additive).""" + conf = RepositoryConf( + name="mozilla-central", + try_name="try", + url="https://hg.mozilla.org/mozilla-central", + try_url="ssh://hg.mozilla.org/try", + decision_env_prefix="GECKO", + ssh_user="reviewbot@mozilla.com", + ) + assert conf.repo_type == "hg" + assert conf._replace(repo_type="git").repo_type == "git" + + +@pytest.mark.parametrize("repo_type, uses_git", [("git", True), ("hg", False)]) +def test_start_analysis_selects_backend( + mock_phabricator, + mock_workflow, + mock_config, + tmpdir, + monkeypatch, + repo_type, + uses_git, +): + """start_analysis picks the Git or Mercurial backend from repo_type.""" + # Import lazily: a module-level import binds taskcluster.utils.stringDate in + # code_review_bot.workflow at collection time, before the autouse + # mock_taskcluster_date fixture can patch it, breaking the date assertions + # of unrelated tests (e.g. test_index.py) + from code_review_bot import workflow as workflow_module + + mock_config.mercurial_cache = tmpdir + mock_config.git_cache = tmpdir + mock_config.ssh_key = "Dummy Private SSH Key" + mock_config.github = {"app_id": 12345, "app_privkey": "AppPrivateKey"} + + # Force the configured repository's backend type + mock_config.repositories = [ + conf._replace(repo_type=repo_type) for conf in mock_config.repositories + ] + + # Build never expires so the analysis proceeds + monkeypatch.setattr(PhabricatorActions, "is_expired_build", lambda _, build: False) + + # Replace both backends with mocks so nothing clones or pushes for real + git_repo, git_worker = mock.MagicMock(), mock.MagicMock() + hg_repo, hg_worker = mock.MagicMock(), mock.MagicMock() + monkeypatch.setattr(workflow_module, "GitRepository", git_repo) + monkeypatch.setattr(workflow_module, "GitWorker", git_worker) + monkeypatch.setattr(workflow_module, "MercurialRepository", hg_repo) + monkeypatch.setattr(workflow_module, "MercurialWorker", hg_worker) + git_worker.return_value.run.return_value = ("success", mock.MagicMock(), {}) + hg_worker.return_value.run.return_value = ("success", mock.MagicMock(), {}) + + # Skip Phabricator/Lando publication of the (mocked) output + mock_workflow.update_build = False + + with mock_phabricator as api: + mock_workflow.phabricator = api + revision = PhabricatorRevision.from_phabricator_trigger( + build_target_phid="PHID-HMBT-test", + phabricator=api, + ) + mock_workflow.start_analysis(revision) + + if uses_git: + assert git_repo.called and git_worker.called + assert not hg_repo.called and not hg_worker.called + # Git path uses the git cache, not the mercurial one + assert git_repo.call_args.kwargs["cache_root"] == mock_config.git_cache + # The GitHub App credentials are passed through + conf = git_repo.call_args.kwargs["config"] + assert conf["github_app_id"] == 12345 + assert conf["github_app_privkey"] == "AppPrivateKey" + else: + assert hg_repo.called and hg_worker.called + assert not git_repo.called and not git_worker.called + assert hg_repo.call_args.kwargs["config"]["ssh_key"] == "Dummy Private SSH Key" + + # Reset settings for following tests + mock_config.mercurial_cache = None + mock_config.git_cache = None + mock_config.ssh_key = None + mock_config.github = {} + + +@pytest.mark.parametrize("missing_base", [False, True]) +def test_publish_analysis_phabricator_git_failure(missing_base): + """A fail:git worker output marks the Phabricator build as failed.""" + build = mock.MagicMock() + build.target_phid = "PHID-HMBT-test" + build.missing_base_revision = missing_base + build.base_revision = "abcdef123456" + + phabricator_api = mock.MagicMock() + payload = ("fail:git", build, {"message": "git apply failed", "duration": 1}) + publish_analysis_phabricator(payload, phabricator_api) + + phabricator_api.update_build_target.assert_called_once() + args, kwargs = phabricator_api.update_build_target.call_args + assert args == ("PHID-HMBT-test", BuildState.Fail) + unit = kwargs["unit"][0] + assert unit["name"] == "git" + assert unit["result"] == "fail" + assert "failed to apply your patch" in unit["details"] + # The missing parent revision is only mentioned when it is the cause + assert ("abcdef123456" in unit["details"]) is missing_base + + +def test_publish_analysis_lando_git_failure(): + """A fail:git worker output publishes the patch failure warning to Lando.""" + build = PhabricatorRevisionBuild(mock.MagicMock(), mock.MagicMock()) + build.revision = {"id": 51} + build.diff_id = 42 + + lando_api = mock.MagicMock() + publish_analysis_lando(("fail:git", build, {}), lando_api) + + lando_api.add_warning.assert_called_once_with(LANDO_FAILURE_HG_MESSAGE, 51, 42) diff --git a/docs/configuration.md b/docs/configuration.md index a4df0b663..0ff52ae65 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -46,6 +46,16 @@ common: api_key: api-xxxx publish: true + # GitHub App credentials used by the bot to push patch stacks to Git try + # repositories (see repo_type below). The App must be installed on the + # target repositories, with "Contents" read & write permission. + GITHUB: + app_id: 123456 + app_privkey: | + -----BEGIN RSA PRIVATE KEY----- + xxxx + -----END RSA PRIVATE KEY----- + repositories: # A unique display name for the repository - name: mozilla-central @@ -67,6 +77,12 @@ common: # - default, to use the default hg clone (for small repositories only) checkout: robust + # (Optional) Version control system of the repository: hg (default) or git + # A git repository is pushed to its try_url over HTTPS, authenticated with + # a short-lived token generated from the GITHUB App credentials, on the + # code-review branch (can be overridden with head_branch) + repo_type: hg + # Prefix of the environment variables used by the bot to detect which repository # is setup from a decision task (more details on the bot documentation) decision_env_prefix: GECKO