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..c2a596220 100644 --- a/bot/code_review_bot/cli.py +++ b/bot/code_review_bot/cli.py @@ -55,6 +55,11 @@ def parse_cli(): help="Taskcluster Secret path", default=os.environ.get("TASKCLUSTER_SECRET"), ) + parser.add_argument( + "--git-ssh-key-secret", + help="Taskcluster secret holding the SSH deploy key used to push to Git try repositories", + default=os.environ.get("GIT_SSH_KEY_SECRET"), + ) parser.add_argument( "--mercurial-repository", help="Optional path to a up-to-date mercurial repository matching the analyzed revision.\n" @@ -116,6 +121,12 @@ def main(): # Setup libmozdata configuration setup_libmozdata("code-review-bot") + # Load the dedicated Git deploy key from its own secret when configured + git_ssh_key = None + if args.git_ssh_key_secret: + secret = taskcluster.get_service("secrets").get(args.git_ssh_key_secret) + git_ssh_key = secret["secret"]["ssh_privkey"] + # Setup settings before stats settings.setup( taskcluster.secrets["APP_CHANNEL"], @@ -124,6 +135,7 @@ def main(): taskcluster.secrets["ssh_key"], args.mercurial_repository, args.github_repository, + git_ssh_key=git_ssh_key, ) # Setup statistics diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py index 1fc398c9a..b27db40fe 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 + # SSH deploy key used to push on Git try repositories + self.git_ssh_key = None + # 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, + git_ssh_key=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.git_ssh_key = git_ssh_key + 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..e8d9d3f8c 100644 --- a/bot/code_review_bot/git.py +++ b/bot/code_review_bot/git.py @@ -1,10 +1,49 @@ +import atexit +import json +import os +import re +import tempfile +import time from urllib.parse import urlparse +import rs_parsepatch import structlog from git import Repo +from git.exc import GitCommandError +from libmozdata.phabricator import PhabricatorPatch + +from code_review_bot.sources.phabricator import PhabricatorBuild logger = structlog.getLogger(__name__) +# Treeherder job URL for a pushed revision. The repository name (first slot) is +# the Treeherder repo, refined for the Git try repository in a later change. +TREEHERDER_URL = "https://treeherder.mozilla.org/#/jobs?repo={}&revision={}" + +# Number of allowed retries on an unexpected push failure, and the base of the +# exponential backoff between them (6s, 36s, 3.6min, 21.6min). Mirrors the +# Mercurial worker, minus the treestatus wait (there is no Git "try" tree). +MAX_PUSH_RETRIES = 4 +PUSH_RETRY_EXPONENTIAL_DELAY = 6 + +# 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*$" +) + + +class RetryNeeded(Exception): + """ + Raised when retrying a Git build is needed + """ + def build_repo_slug(repo_url): """ @@ -72,3 +111,450 @@ def git_clone(base_repository, head_repository, revision, destination): repo.head.reference = repo.commit(revision) return repo + + +class GitRepository: + """ + A Git repository with credentials to push a patch stack to a remote + (e.g. a GitHub "try" repository). + + Mirrors the interface of the Mercurial ``Repository`` + (``code_review_bot.mercurial.Repository``) so the same workflow can apply a + Phabricator patch stack and push it, but targeting Git instead of Mercurial. + + Notable 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 passed through ``GIT_SSH_COMMAND``. + """ + + def __init__(self, config, cache_root): + assert isinstance(config, dict) + self.name = config["name"] + self.url = config["url"] + self.dir = os.path.join(cache_root, config["name"]) + self.try_url = config["try_url"] + self.try_name = config.get("try_name", "try") + + # Revision/branch to apply patches on when the base is unknown locally + self.default_revision = config.get("default_revision", "HEAD") + + # Branch pushed to the remote try repository. + # TODO: the per-build ref model is still an open question (see plan Q2). + self.head_branch = config.get("head_branch", "code-review") + + # Apply patches on the latest revision when True + self.use_latest_revision = config.get("use_latest_revision", False) + + self._repo = None + + # Write the SSH (deploy) key from the configuration to a temporary file + _, self.ssh_key_path = tempfile.mkstemp(suffix=".key") + with open(self.ssh_key_path, "w") as f: + f.write(config["ssh_key"]) + os.chmod(self.ssh_key_path, 0o600) + self.git_ssh_command = f"ssh -i {self.ssh_key_path} -o StrictHostKeyChecking=no" + + # Remove the key when finished + atexit.register(self.end_of_life) + + def __str__(self): + return self.name + + def end_of_life(self): + if os.path.exists(self.ssh_key_path): + os.unlink(self.ssh_key_path) + logger.info("Removed ssh key") + + @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 clone(self): + 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) + with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): + self.repo.remotes.origin.fetch() + else: + self._repo = Repo.clone_from( + self.url, self.dir, env={"GIT_SSH_COMMAND": self.git_ssh_command} + ) + 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 + + @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 commit_patch(self, patch_content, message, name, email): + """Apply a single unified diff to the index and commit it.""" + with tempfile.NamedTemporaryFile( + mode="w", suffix=".diff", delete=False + ) as patch_file: + patch_file.write(self.normalize_patch(patch_content)) + 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 apply_build(self, build): + """Apply a stack of Phabricator patches as Git commits.""" + assert isinstance(build, PhabricatorBuild) + assert len(build.stack) > 0, "No patches to apply" + assert all(isinstance(p, PhabricatorPatch) for p in build.stack) + + # Find the first unknown base revision + needed_stack = [] + for patch in reversed(build.stack): + # Skip already merged patches + if patch.merged: + logger.info( + f"Skip applying patch {patch.id} as it's already been merged upstream" + ) + continue + + # Add the patch into the stack only if not already merged + needed_stack.insert(0, patch) + + # Stop as soon as a base revision is available + if self.has_revision(patch.base_revision): + logger.info(f"Stopping at revision {patch.base_revision}") + break + + if not needed_stack: + logger.info("All the patches are already applied") + return + + git_base = self.get_base_identifier(needed_stack) + + # When base revision is missing, fall back to the default revision + build.base_revision = git_base + build.missing_base_revision = not self.has_revision(git_base) + if build.missing_base_revision: + logger.warning( + "Missing base revision from Phabricator", + revision=git_base, + fallback=self.default_revision, + ) + git_base = self.default_revision + + # Store the actual base revision we used + build.actual_base_revision = git_base + + # Move the working tree to the base revision. Detach HEAD so the patches + # we commit on top stay throwaway drafts and never advance a branch ref; + # clean() can then discard them simply by returning to the base. + logger.info(f"Updating repo to revision {git_base}") + self.repo.git.checkout(git_base, force=True, detach=True) + + for patch in needed_stack: + if patch.commits: + # Use the first commit only + commit = patch.commits[0] + message = "{}\n".format(commit["message"]) + name, email = self.get_author(commit) + else: + # We should always have some commits here + logger.warning("Missing commit on patch", id=patch.id) + message = "" + name, email = DEFAULT_AUTHOR_NAME, DEFAULT_AUTHOR_EMAIL + message += f"Differential Diff: {patch.phid}" + + logger.info("Applying patch", phid=patch.phid, message=message) + self.commit_patch(patch.patch, message, name, email) + + def add_try_commit(self, build): + """ + Build and commit the file configuring try with try_task_config.json + and the code-review workflow parameters in JSON + """ + path = os.path.join(self.dir, "try_task_config.json") + config = { + "version": 2, + "parameters": { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + "phabricator_diff": build.target_phid, + }, + } + diff_phid = build.stack[-1].phid + + if build.revision_url: + message = f"try_task_config for {build.revision_url}" + else: + message = "try_task_config for code-review" + message += f"\nDifferential Diff: {diff_phid}" + + # Write content as json and commit it + with open(path, "w") as f: + json.dump(config, f, sort_keys=True, indent=4) + + 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 over the deploy key.""" + head = self.repo.head.commit + logger.info("Pushing patches to try", rev=head.hexsha, branch=self.head_branch) + with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): + self.repo.git.push( + self.try_url, f"HEAD:refs/heads/{self.head_branch}", force=True + ) + return head + + 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. Because ``apply_build`` + commits on a detached HEAD, returning to the base leaves those commits + unreferenced instead of accumulating them on a branch. + """ + 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): + with self.repo.git.custom_environment(GIT_SSH_COMMAND=self.git_ssh_command): + 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: + """ + Drive a GitRepository through a single build: clean, apply the patch stack, + push to the remote try repository and return a Treeherder link. + + Mirrors ``code_review_bot.mercurial.MercurialWorker`` but for Git. The key + difference is that there is no treestatus wait before retrying: Git has no + "try" tree to gate on, so failed pushes are simply retried with backoff. + """ + + 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 __init__(self, skippable_files=[]): + self.skippable_files = skippable_files + + def run(self, repository, build): + """ + Apply the stack of patches from the build, retrying on remote push + errors. Unlike the Mercurial worker, there is no treestatus wait. + """ + while build.retries <= MAX_PUSH_RETRIES: + start = time.time() + + if build.retries: + logger.warning( + "Trying to apply build's diff after a remote push error " + f"[{build.retries}/{MAX_PUSH_RETRIES}]" + ) + + try: + return self.handle_build(repository, build) + except RetryNeeded: + build.retries += 1 + + if build.retries > MAX_PUSH_RETRIES: + error_log = "Max number of retries has been reached pushing the build to try repository" + logger.warn("Git error on diff", error=error_log, build=build) + return ( + "fail:git", + build, + {"message": error_log, "duration": time.time() - start}, + ) + + # Wait an exponential time before retrying the build + delay = PUSH_RETRY_EXPONENTIAL_DELAY**build.retries + logger.info( + f"An error occurred pushing the build to try, retrying after {delay}s" + ) + time.sleep(delay) + + def is_commit_skippable(self, build): + def get_files_touched_in_diff(rawdiff): + patched = [] + for parsed_diff in rs_parsepatch.get_diffs(rawdiff): + # filename is sometimes of format 'test.txt Tue Feb 05 17:23:40 2019 +0100' + # fix after https://github.com/mozilla/rust-parsepatch/issues/61 + if "filename" in parsed_diff: + filename = parsed_diff["filename"].split(" ")[0] + patched.append(filename) + return patched + + return any( + patched_file in self.skippable_files + for rev in build.stack + for patched_file in get_files_touched_in_diff(rev.patch) + ) + + def is_eligible_for_retry(self, error): + """ + Given a Git error message, if it's likely due to a temporary connection + problem, consider it eligible for retry. + """ + error = error.lower() + return any( + eligible_message in error for eligible_message in self.ELIGIBLE_RETRY_ERRORS + ) + + def handle_build(self, repository, build): + """ + Apply the build's diff on the local clone; on success push to try and + return a treeherder link. Unexpected push failures raise RetryNeeded so + run() retries; other failures return a warning result. + """ + assert isinstance(repository, GitRepository) + start = time.time() + + try: + # Start by cleaning the repo + repository.clean() + + # First apply patches on local repo + repository.apply_build(build) + + # Check Eligibility: some commits don't need to be pushed to try. + if self.is_commit_skippable(build): + logger.info("This patch series is ineligible for automated try push") + return ( + "fail:ineligible", + build, + { + "message": "Modified files match skippable internal configuration files", + "duration": time.time() - start, + }, + ) + + # Configure the try task + repository.add_try_commit(build) + + # Then push that stack on try + tip = repository.push_to_try() + logger.info("Diff has been pushed !") + + # Publish Treeherder link + uri = TREEHERDER_URL.format(repository.try_name, tip.hexsha) + except GitCommandError as e: + error_log = e.stderr or str(e) + + if self.is_eligible_for_retry(error_log): + raise RetryNeeded + + logger.warn("Git error on diff", error=error_log, args=e.args, build=build) + return ( + "fail:git", + build, + {"message": error_log, "duration": time.time() - start}, + ) + + except Exception as e: + logger.warn("Failed to process diff", error=e, build=build) + return ( + "fail:general", + build, + {"message": str(e), "duration": time.time() - start}, + ) + + return ( + "success", + build, + {"treeherder_url": uri, "revision": tip.hexsha}, + ) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 2655f6fd0..a4c770444 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 MercurialWorker, Repository, robust_checkout from code_review_bot.report.debug import DebugReporter from code_review_bot.revisions import GithubRevision, PhabricatorRevision, Revision @@ -272,9 +272,9 @@ 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 an ssh key + if not settings.ssh_key and not settings.git_ssh_key: + raise Exception("An SSH key must be configured to start analysis") # Set the Phabricator build as running self.update_status(revision, state=BuildState.Work) @@ -295,21 +295,38 @@ def start_analysis(self, revision): api_key=self.phabricator.api_key, ) - # Initialize mercurial repository - repository = Repository( - 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, + # Deploy key from the dedicated secret, fallback to the global key + "ssh_key": settings.git_ssh_key or settings.ssh_key, + }, + cache_root=settings.git_cache, + ) + worker = GitWorker() + else: + repository = Repository( + 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): @@ -340,7 +357,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/tests/conftest.py b/bot/tests/conftest.py index 56bf2d1ff..f1448becf 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -1191,6 +1191,63 @@ 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", + "ssh_key": "privateSSHkey", + "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..c5ecca521 --- /dev/null +++ b/bot/tests/test_git.py @@ -0,0 +1,332 @@ +# 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 MAX_PUSH_RETRIES, GitRepository, GitWorker + +# 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()), + "ssh_key": "privateSSHkey", + } + 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_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.git.time.sleep", lambda *a, **k: None) + + worker = GitWorker() + + # The Git worker has no treestatus gate at all + assert not hasattr(worker, "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..1b09ea1b4 100644 --- a/bot/tests/test_phabricator_analysis.py +++ b/bot/tests/test_phabricator_analysis.py @@ -5,12 +5,16 @@ import json import tempfile from unittest import mock +from unittest.mock import MagicMock 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 +274,131 @@ 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) + + +@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) + + +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_ssh_key", + [ + ("git", True, "GitDeployKey"), + ("git", True, None), + ("hg", False, None), + ], +) +def test_start_analysis_selects_backend( + mock_phabricator, + mock_workflow, + mock_config, + tmpdir, + monkeypatch, + repo_type, + uses_git, + git_ssh_key, +): + """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.git_ssh_key = git_ssh_key + + # 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 = MagicMock(), MagicMock() + hg_repo, hg_worker = MagicMock(), MagicMock() + monkeypatch.setattr(workflow_module, "GitRepository", git_repo) + monkeypatch.setattr(workflow_module, "GitWorker", git_worker) + monkeypatch.setattr(workflow_module, "Repository", hg_repo) + monkeypatch.setattr(workflow_module, "MercurialWorker", hg_worker) + git_worker.return_value.run.return_value = ("success", MagicMock(), {}) + hg_worker.return_value.run.return_value = ("success", 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 dedicated deploy key wins, falling back to the global key + expected_key = git_ssh_key or "Dummy Private SSH Key" + assert git_repo.call_args.kwargs["config"]["ssh_key"] == expected_key + 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.git_ssh_key = None