From a9cb7ca77fcf2d4d5e9890c77fc52820a6aabe2a Mon Sep 17 00:00:00 2001 From: Bastien Orivel Date: Wed, 15 Apr 2026 15:12:54 +0200 Subject: [PATCH] Move git notes fetching from run-task to the decision task and make them optional This removes the concept of extra refs fetching as it was introduced for this feature alone as it turned out to be quite complex to support if we wanted notes to be optional. The main issue it's fixing is being able to run decision tasks off of repos that don't have that notes branch at all. --- .taskcluster.yml | 1 - src/taskgraph/decision.py | 2 +- src/taskgraph/run-task/run-task | 15 +---------- src/taskgraph/util/vcs.py | 35 ++++++++++++++++++++++--- test/test_decision.py | 4 ++- test/test_scripts_run_task.py | 46 --------------------------------- test/test_util_vcs.py | 20 +++++++------- 7 files changed, 47 insertions(+), 76 deletions(-) diff --git a/.taskcluster.yml b/.taskcluster.yml index 2899508d6..dbc15a8e6 100644 --- a/.taskcluster.yml +++ b/.taskcluster.yml @@ -226,7 +226,6 @@ tasks: - $if: 'isPullRequest' then: TASKGRAPH_PULL_REQUEST_NUMBER: '${event.pull_request.number}' - TASKGRAPH_EXTRA_REFS: {$json: ["refs/notes/decision-parameters"]} - $if: 'tasks_for == "action" || tasks_for == "pr-action"' then: ACTION_TASK_GROUP_ID: '${action.taskGroupId}' # taskGroupId of the target task diff --git a/src/taskgraph/decision.py b/src/taskgraph/decision.py index b5ebb9e7c..4a35ca2d1 100644 --- a/src/taskgraph/decision.py +++ b/src/taskgraph/decision.py @@ -267,7 +267,7 @@ def get_decision_parameters(graph_config, options): # load extra parameters from vcs note if able note_ref = "refs/notes/decision-parameters" if options.get("allow_parameter_override") and ( - note_params := repo.get_note(note_ref) + note_params := repo.get_note(note_ref, parameters["head_repository"]) ): try: note_params = json.loads(note_params) diff --git a/src/taskgraph/run-task/run-task b/src/taskgraph/run-task/run-task index 85aa29815..deca3a691 100755 --- a/src/taskgraph/run-task/run-task +++ b/src/taskgraph/run-task/run-task @@ -31,7 +31,7 @@ import time import urllib.error import urllib.request from pathlib import Path -from typing import Dict, List, Optional +from typing import Dict, Optional SECRET_BASEURL_TPL = "{}/secrets/v1/secret/{{}}".format( os.environ.get("TASKCLUSTER_PROXY_URL", "http://taskcluster").rstrip("/") @@ -640,7 +640,6 @@ def git_checkout( ssh_key_file: Optional[Path], ssh_known_hosts_file: Optional[Path], shallow: bool = False, - extra_refs: Optional[List[str]] = None, ): assert head_ref or head_rev @@ -735,14 +734,6 @@ def git_checkout( env=env, ) - if extra_refs: - git_fetch( - destination_path, - *[f"{ref}:{ref}" for ref in extra_refs], - remote=head_repo, - env=env, - ) - args = [ "git", "checkout", @@ -941,8 +932,6 @@ def collect_vcs_options(args, project, name): head_rev = os.environ.get(f"{env_prefix}_HEAD_REV") pip_requirements = os.environ.get(f"{env_prefix}_PIP_REQUIREMENTS") private_key_secret = os.environ.get(f"{env_prefix}_SSH_SECRET_NAME") - if extra_refs := os.environ.get(f"{env_prefix}_EXTRA_REFS"): - extra_refs = json.loads(extra_refs) store_path = os.environ.get("HG_STORE_PATH") @@ -976,7 +965,6 @@ def collect_vcs_options(args, project, name): "ssh-secret-name": private_key_secret, "pip-requirements": pip_requirements, "shallow-clone": shallow_clone, - "extra-refs": extra_refs, } @@ -1026,7 +1014,6 @@ def vcs_checkout_from_args(options): ssh_key_file, ssh_known_hosts_file, shallow=options.get("shallow-clone", False), - extra_refs=options.get("extra-refs"), ) elif options["repo-type"] == "hg": revision = hg_checkout( diff --git a/src/taskgraph/util/vcs.py b/src/taskgraph/util/vcs.py index 584b166f9..d54a81acf 100644 --- a/src/taskgraph/util/vcs.py +++ b/src/taskgraph/util/vcs.py @@ -215,9 +215,17 @@ def does_revision_exist_locally(self, revision: str) -> bool: If this function returns an unexpected value, then make sure the revision was fetched from the remote repository.""" - def get_note(self, note: str, revision: Optional[str] = None) -> Optional[str]: + def get_note( + self, + note: str, + remote: str, + revision: Optional[str] = None, + ) -> Optional[str]: """Read a note attached to the given revision (defaults to HEAD). + The notes ref is fetched from ``remote`` first; a missing ref on the + remote is tolerated while other fetch failures propagate. + Returns the note content as a string, or ``None`` if no note exists. Only supported by Git; returns ``None`` for all other VCS types. """ @@ -594,13 +602,32 @@ def does_revision_exist_locally(self, revision): return False raise - def get_note(self, note: str, revision: Optional[str] = None) -> Optional[str]: + def get_note( + self, + note: str, + remote: str, + revision: Optional[str] = None, + ) -> Optional[str]: if not note.startswith("refs/notes/"): note = f"refs/notes/{note}" - revision = revision or "HEAD" try: - return self.run("notes", f"--ref={note}", "show", revision).strip() + self.run("fetch", remote, f"{note}:{note}") + except subprocess.CalledProcessError: + ls = subprocess.run( + ["git", "ls-remote", "--exit-code", remote, note], + cwd=self.path, + capture_output=True, + ) + # If the note doesn't exist, ignore the fetch failure + if ls.returncode == 2: + return None + raise + + try: + return self.run( + "notes", f"--ref={note}", "show", revision or "HEAD" + ).strip() except subprocess.CalledProcessError as e: if e.returncode == 1: return None diff --git a/test/test_decision.py b/test/test_decision.py index 2d9b23baa..677d9a9b1 100644 --- a/test/test_decision.py +++ b/test/test_decision.py @@ -167,7 +167,9 @@ def test_dontbuild_commit_message_yields_default_target_tasks_method( def test_decision_parameters_note(mock_files_changed, mock_get_note): options = {**_BASE_OPTIONS, "allow_parameter_override": True} params = decision.get_decision_parameters(FAKE_GRAPH_CONFIG, options) - mock_get_note.assert_called_once_with("refs/notes/decision-parameters") + mock_get_note.assert_called_once_with( + "refs/notes/decision-parameters", _BASE_OPTIONS["head_repository"] + ) assert params["build_number"] == 99 diff --git a/test/test_scripts_run_task.py b/test/test_scripts_run_task.py index 0d60a8d41..b2393721c 100644 --- a/test/test_scripts_run_task.py +++ b/test/test_scripts_run_task.py @@ -1,5 +1,4 @@ import functools -import json import os import site import stat @@ -180,17 +179,6 @@ def test_install_pip_requirements_with_uv( {"shallow-clone": True}, id="git_with_shallow_clone", ), - pytest.param( - {}, - { - "REPOSITORY_TYPE": "git", - "HEAD_REPOSITORY": "https://github.com/example/repo", - "HEAD_REV": "abc123", - "EXTRA_REFS": json.dumps(["refs/notes/taskgraph", "refs/notes/other"]), - }, - {"extra-refs": ["refs/notes/taskgraph", "refs/notes/other"]}, - id="git_with_extra_refs", - ), ], ) def test_collect_vcs_options( @@ -228,7 +216,6 @@ def test_collect_vcs_options( "shallow-clone": False, "ssh-secret-name": env.get("SSH_SECRET_NAME"), "store-path": env.get("HG_STORE_PATH"), - "extra-refs": None, } if "PIP_REQUIREMENTS" in env: expected["pip-requirements"] = os.path.join( @@ -651,36 +638,3 @@ def test_main_abspath_environment(mocker, run_main): assert env.get("MOZ_UV_HOME") == "/builds/worker/dir/uv" for key in envvars: assert env[key] == "/builds/worker/file" - - -def test_git_checkout_extra_refs(mock_stdin, run_task_mod, mock_git_repo, tmp_path): - """extra_refs are fetched into the local repo during checkout.""" - # Add a notes ref to the source repo - rev = mock_git_repo["main"][-1] - subprocess.check_call( - ["git", "notes", "--ref=refs/notes/taskgraph", "add", "-m", "test", rev], - cwd=mock_git_repo["path"], - ) - - destination = tmp_path / "destination" - run_task_mod.git_checkout( - destination_path=str(destination), - head_repo=mock_git_repo["path"], - base_repo=mock_git_repo["path"], - base_rev=None, - head_ref="main", - head_rev=None, - ssh_key_file=None, - ssh_known_hosts_file=None, - extra_refs=["refs/notes/taskgraph"], - ) - - # Verify the notes ref is available locally - result = subprocess.run( - ["git", "notes", "--ref=refs/notes/taskgraph", "show", rev], - cwd=str(destination), - capture_output=True, - text=True, - ) - assert result.returncode == 0 - assert "test" in result.stdout diff --git a/test/test_util_vcs.py b/test/test_util_vcs.py index 84b97940d..0d36c21f5 100644 --- a/test/test_util_vcs.py +++ b/test/test_util_vcs.py @@ -653,19 +653,21 @@ def test_get_changed_files_with_null_base_revision_shallow_clone( def test_get_note_git(git_repo, tmpdir): """get_note returns note content when present, None otherwise.""" - repo_path = tmpdir.join("git") - shutil.copytree(git_repo, repo_path) - repo = get_repository(str(repo_path)) + src_path = str(tmpdir.join("git-src")) + dst_path = str(tmpdir.join("git-dst")) + shutil.copytree(git_repo, src_path) + shutil.copytree(git_repo, dst_path) + repo = get_repository(dst_path) - # No note yet - assert repo.get_note("try-config") is None + # No note yet on the remote + assert repo.get_note("try-config", src_path) is None rev = repo.head_rev subprocess.check_call( ["git", "notes", "--ref=refs/notes/try-config", "add", "-m", "test note", rev], - cwd=repo.path, + cwd=src_path, ) - assert repo.get_note("try-config") == "test note" - assert repo.get_note("try-config", rev) == "test note" - assert repo.get_note("other") is None + assert repo.get_note("try-config", src_path) == "test note" + assert repo.get_note("try-config", src_path, revision=rev) == "test note" + assert repo.get_note("other", src_path) is None