From efcf5a1cb7dec495bea84ae3f23a595d39f0ebb9 Mon Sep 17 00:00:00 2001 From: Caue Santos Date: Wed, 21 Oct 2026 10:33:25 -0600 Subject: [PATCH 1/7] feat(backport-fix-imports): rewrite module paths on backports to older module majors --- .../actions/backport-fix-imports/README.md | 95 +++++ .../actions/backport-fix-imports/action.yml | 64 ++++ .../actions/backport-fix-imports/src/fix.py | 326 +++++++++++++++++ .../backport-fix-imports/test/test_fix.py | 329 ++++++++++++++++++ .../workflows/test-backport-fix-imports.yaml | 27 ++ Makefile | 7 +- README.md | 8 + 7 files changed, 854 insertions(+), 2 deletions(-) create mode 100644 .github/actions/backport-fix-imports/README.md create mode 100644 .github/actions/backport-fix-imports/action.yml create mode 100644 .github/actions/backport-fix-imports/src/fix.py create mode 100644 .github/actions/backport-fix-imports/test/test_fix.py create mode 100644 .github/workflows/test-backport-fix-imports.yaml diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md new file mode 100644 index 00000000..42bc7cd4 --- /dev/null +++ b/.github/actions/backport-fix-imports/README.md @@ -0,0 +1,95 @@ +# Backport fix imports + +Repairs Go module paths on a backport branch when the target release line is +on an older module major than the source. The case it was built for: +`loft-enterprise` moved `main` to `github.com/loft-sh/loft/v5` and +`github.com/loft-sh/api/v5`, while every `release-4.x` branch stays on v4. A +cherry-pick from `main` then carries v5 imports into a v4 branch, and conflicts +wherever both sides edited the same import block. + +## What it does + +Run inside a full-history checkout of the repository, on a backport branch that +already exists on the remote: + +1. Reads the root `go.mod` on the source and target branches and maps the + modules the repository owns: the `module` line, and `replace` directives + that point into the repository (`=> ./...`). Third-party modules are never + rewritten, so a dependency major bump on `main` is left alone. +2. Moves files the backport added under a source-only local directory (for + example `staging/src/github.com/loft-sh/api/v5/...`) into the target's + directory. +3. Resolves conflict hunks made only of import lines inside an import block. + The backported side wins, rewritten to the target paths. An import only the + target side had stays while the file still uses it. +4. Rewrites the mapped paths in the `.go` files the backport changed, only at + the start of a string literal and on a path boundary. +5. Runs `gofmt` on files whose import blocks it resolved, then commits as + `github-actions[bot]` and pushes one commit: + `fix(backport): use module paths`. + +Conflict hunks in real code are left as they are and counted in +`conflicts-remaining`, with a warning per file. A second run finds nothing to +rewrite and reports `unchanged`, so re-runs are safe. + +The action never fails the job: every failure path ends with `conclusion=failed`, +a one-line `reason` and exit 0, and nothing is pushed. + +## Usage + +```yaml +- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + persist-credentials: false + +- id: fix + uses: loft-sh/github-actions/.github/actions/backport-fix-imports@backport-fix-imports/v1 + with: + target-branch: release-4.12 + backport-branch: backport/release-4.12/pr-8606 + github-token: ${{ secrets.GH_ACCESS_TOKEN }} + +- if: steps.fix.outputs.conflicts-remaining != '0' + run: echo "conflicts left for review" +``` + +The runner needs `python3` and `git`; `gofmt` is optional and only formats the +import blocks it resolved (it ships on GitHub-hosted Ubuntu runners). Pushing +with a PAT, rather than `GITHUB_TOKEN`, is what makes the fix commit trigger +the backport PR's CI. + + + +| INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | +|-----------------|--------|----------|----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| backport-branch | string | true | | Head branch of the backport PR,
e.g. backport/release-4.12/pr-8606. Checked out, fixed and
pushed back. | +| github-token | string | true | | Token with contents write on the
repository, used to fetch and push
the backport branch. Supplied to git
through the environment, never argv or
.git/config. | +| push | string | false | `"true"` | false commits the fix locally without
pushing (dry run). | +| source-branch | string | false | `"main"` | Branch the original change merged into.
Its root go.mod gives the source
module paths. | +| target-branch | string | true | | Release branch the backport PR targets,
e.g. release-4.12. Its root go.mod gives
the target module paths. | + + + + + +| OUTPUT | TYPE | DESCRIPTION | +|---------------------|--------|-------------------------------------------------------------------------------------------------------------------------------------| +| commit | string | SHA of the fix commit; empty
when none was made. | +| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | +| conflicts-remaining | string | Number of changed files that still
carry conflict markers after the fix. | +| files-changed | string | Number of .go files rewritten. | +| hunks-resolved | string | Number of import-only conflict hunks resolved. | +| reason | string | One-line explanation of the conclusion. | + + + +## Testing + +```bash +make test-backport-fix-imports +``` + +Runs the pytest suite in `test/`: the `go.mod` mapping, the import-hunk +resolver, and full runs against throwaway git repos pushed to a local bare +remote. No network and no tokens. diff --git a/.github/actions/backport-fix-imports/action.yml b/.github/actions/backport-fix-imports/action.yml new file mode 100644 index 00000000..e513b83e --- /dev/null +++ b/.github/actions/backport-fix-imports/action.yml @@ -0,0 +1,64 @@ +name: Backport fix imports +description: | + Repair Go module paths on a backport branch whose target release line is on + an older module major than its source (for example a v5 main backported to a + v4 release branch). Rewrites the import paths of modules the repository owns, + moves files added under a source-only staging directory, resolves conflict + hunks made only of import lines, then commits and pushes one commit. Conflicts + in real code are left and counted. Advisory: every failure path warns and + exits 0, so it never fails the backport that calls it. +inputs: + target-branch: + description: 'Release branch the backport PR targets, e.g. release-4.12. Its root go.mod gives the target module paths.' + required: true + backport-branch: + description: 'Head branch of the backport PR, e.g. backport/release-4.12/pr-8606. Checked out, fixed and pushed back.' + required: true + source-branch: + description: 'Branch the original change merged into. Its root go.mod gives the source module paths.' + required: false + default: 'main' + github-token: + description: 'Token with contents write on the repository, used to fetch and push the backport branch. Supplied to git through the environment, never argv or .git/config.' + required: true + push: + description: 'false commits the fix locally without pushing (dry run).' + required: false + default: 'true' + +outputs: + conclusion: + description: 'fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input) or failed.' + value: ${{ steps.fix.outputs.conclusion }} + reason: + description: 'One-line explanation of the conclusion.' + value: ${{ steps.fix.outputs.reason }} + files-changed: + description: 'Number of .go files rewritten.' + value: ${{ steps.fix.outputs.files-changed }} + hunks-resolved: + description: 'Number of import-only conflict hunks resolved.' + value: ${{ steps.fix.outputs.hunks-resolved }} + conflicts-remaining: + description: 'Number of changed files that still carry conflict markers after the fix.' + value: ${{ steps.fix.outputs.conflicts-remaining }} + commit: + description: 'SHA of the fix commit; empty when none was made.' + value: ${{ steps.fix.outputs.commit }} + +runs: + using: composite + steps: + # The script exits 0 on every path it controls; continue-on-error covers + # the rest (a missing interpreter), so a backport is never failed by it. + - name: Fix module paths + id: fix + continue-on-error: true + shell: bash + env: + TARGET_BRANCH: ${{ inputs.target-branch }} + BACKPORT_BRANCH: ${{ inputs.backport-branch }} + SOURCE_BRANCH: ${{ inputs.source-branch }} + PUSH: ${{ inputs.push }} + GH_TOKEN: ${{ inputs.github-token }} + run: python3 "${{ github.action_path }}/src/fix.py" diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py new file mode 100644 index 00000000..da20bcbf --- /dev/null +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -0,0 +1,326 @@ +#!/usr/bin/env python3 +"""backport-fix-imports: repair Go module paths on a backport branch whose +target release line is on an older module major than its source. + +A cherry-pick from a v5 main onto a v4 release branch carries v5 import +paths with it, and conflicts wherever both sides edited an import block. +This script, run inside a full-history checkout of the repository: + + 1. derives the path mapping from go.mod on both branches (only modules the + repository owns: the module line and replaces that point into the repo) + 2. moves files the backport added under a source-only staging directory + into the target's directory + 3. resolves conflict hunks that contain nothing but import lines + 4. rewrites the mapped import paths in the .go files the backport changed + 5. formats the files it resolved, commits as one commit and pushes + +Conflicts in real code are left as they are and counted, for a person or a +later AI pass. A second run finds nothing to do, so re-runs are no-ops. + +The action is advisory: every path, including an unexpected exception, ends +with exit 0 and a conclusion of fixed, unchanged, skipped or failed. + +Env: + TARGET_BRANCH release branch the backport PR targets (required) + SOURCE_BRANCH branch the original change merged into (default main) + BACKPORT_BRANCH head branch of the backport PR (required) + REMOTE git remote to fetch from and push to (default origin) + PUSH "false" commits locally without pushing (default true) + GH_TOKEN token used for fetch and push; never logged + GITHUB_OUTPUT standard Actions output file +""" +from __future__ import annotations + +import base64 +import os +import re +import shutil +import subprocess +import sys +from pathlib import Path + +MARK_OURS = "<<<<<<< " +MARK_BASE = "||||||| " +MARK_SEP = "=======" +MARK_THEIRS = ">>>>>>> " + +GOMOD_MODULE = re.compile(r"^module\s+(\S+)", re.M) +# Single-line replaces and the lines of a replace ( ... ) block; only a local +# right-hand side ("./" or "../") names a module this repository owns. +GOMOD_REPLACE = re.compile(r"^[ \t]*(?:replace[ \t]+)?(\S+)(?:[ \t]+\S+)?[ \t]+=>[ \t]+(\.\.?/\S+)[ \t]*$", re.M) +MAJOR_SUFFIX = re.compile(r"/v\d+$") + +# One import spec per line: optional name, quoted path, optional comment. +IMPORT_SPEC = re.compile(r'^\s*(?:([A-Za-z_][A-Za-z0-9_]*|\.)\s+)?"([^"\s]+)"\s*(?://.*)?$') +COMMENT = re.compile(r"^\s*//") +IDENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") + +BOT_NAME = "github-actions[bot]" +BOT_EMAIL = "41898282+github-actions[bot]@users.noreply.github.com" + + +class Skip(Exception): + """An expected condition: nothing to fix here.""" + + +def emit(key: str, value: str) -> None: + with open(os.environ.get("GITHUB_OUTPUT", os.devnull), "a", encoding="utf-8") as f: + f.write(f"{key}={value}\n") + + +def git(*args: str, check: bool = True) -> str: + env = os.environ.copy() + token = os.environ.get("GH_TOKEN", "") + if token: + # The PAT goes in through the environment, never argv or .git/config. + auth = base64.b64encode(f"x-access-token:{token}".encode()).decode() + env.update(GIT_CONFIG_COUNT="1", GIT_CONFIG_KEY_0="http.extraheader", + GIT_CONFIG_VALUE_0=f"AUTHORIZATION: basic {auth}") + res = subprocess.run(["git", *args], capture_output=True, text=True, env=env) + if check and res.returncode != 0: + raise RuntimeError(f"git {args[0]} failed: {res.stderr.strip()}") + return res.stdout + + +def strip_major(path: str) -> str: + return MAJOR_SUFFIX.sub("", path) + + +def owned_modules(gomod: str) -> dict[str, tuple[str, str]]: + """Map each owned module, keyed by its path without the major suffix, to + (full path, local directory or "").""" + owned: dict[str, tuple[str, str]] = {} + m = GOMOD_MODULE.search(gomod) + if m: + owned[strip_major(m.group(1))] = (m.group(1), "") + for path, local in GOMOD_REPLACE.findall(gomod): + owned[strip_major(path)] = (path, local.removeprefix("./").rstrip("/")) + return owned + + +def module_mapping(source_gomod: str, target_gomod: str) -> tuple[list[tuple[str, str]], list[tuple[str, str]]]: + """Return (import path pairs, directory pairs) that differ between source and target.""" + source, target = owned_modules(source_gomod), owned_modules(target_gomod) + paths, dirs = [], [] + for base, (src_path, src_dir) in source.items(): + if base not in target: + continue + tgt_path, tgt_dir = target[base] + if src_path != tgt_path: + paths.append((src_path, tgt_path)) + if src_dir and tgt_dir and src_dir != tgt_dir: + dirs.append((src_dir, tgt_dir)) + # Longest first, so a nested module is rewritten before its parent. + paths.sort(key=lambda p: len(p[0]), reverse=True) + return paths, dirs + + +def rewrite_paths(text: str, paths: list[tuple[str, str]]) -> str: + # Only at the start of a string literal and on a path boundary, so + # loft/v5 never matches loft/v50 or a mention in prose. + for src, tgt in paths: + text = re.sub(r'(?<=")' + re.escape(src) + r'(?=["/])', tgt, text) + return text + + +def import_name(line: str) -> str | None: + """Package name an import line binds, or None when it cannot be told + (blank and dot imports, paths whose name is not their last element).""" + m = IMPORT_SPEC.match(line) + if not m or m.group(1) in (".", "_"): + return None + if m.group(1): + return m.group(1) + parts = m.group(2).split("/") + last = parts[-1] + if re.fullmatch(r"v\d+", last) and len(parts) > 1: + last = parts[-2] + last = re.sub(r"\.v\d+$", "", last) + return last if IDENT.fullmatch(last) else None + + +def is_import_only(lines: list[str]) -> bool: + return all(not l.strip() or COMMENT.match(l) or IMPORT_SPEC.match(l) for l in lines) + + +def in_import_block(lines: list[str], index: int) -> bool: + for line in reversed(lines[:index]): + stripped = line.strip() + if stripped == "import (": + return True + if stripped == ")" or stripped.startswith(("func ", "type ", "var ", "const ")): + return False + return False + + +def block_end(lines: list[str], index: int) -> int: + for i in range(index, len(lines)): + if lines[i].strip() == ")": + return i + return len(lines) + + +def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str]: + """Theirs (the backported change) wins. An import only the target side + has stays while the file still uses it, and whenever its name cannot be + told: an unused import fails the build as surely as a missing one.""" + merged = list(theirs) + seen = {m.group(2) for m in map(IMPORT_SPEC.match, theirs) if m} + for line in ours: + m = IMPORT_SPEC.match(line) + if not m or m.group(2) in seen: + continue + name = import_name(line) + if name is None or re.search(r"\b" + re.escape(name) + r"\.", body): + merged.append(line) + seen.add(m.group(2)) + return [l for l in merged if l.strip()] + + +def resolve_import_hunks(text: str, paths: list[tuple[str, str]]) -> tuple[str, int]: + """Resolve conflict hunks made only of import lines inside an import + block. Returns the new text and how many hunks were resolved.""" + lines = text.split("\n") + out: list[str] = [] + resolved = 0 + i = 0 + while i < len(lines): + if not lines[i].startswith(MARK_OURS): + out.append(lines[i]) + i += 1 + continue + try: + sep = next(j for j in range(i + 1, len(lines)) if lines[j] == MARK_SEP) + end = next(j for j in range(sep + 1, len(lines)) if lines[j].startswith(MARK_THEIRS)) + except StopIteration: + out.extend(lines[i:]) + break + base = next((j for j in range(i + 1, sep) if lines[j].startswith(MARK_BASE)), sep) + ours = [rewrite_paths(l, paths) for l in lines[i + 1:base]] + theirs = [rewrite_paths(l, paths) for l in lines[sep + 1:end]] + if is_import_only(ours) and is_import_only(theirs) and in_import_block(lines, i): + body = "\n".join(lines[block_end(lines, end + 1) + 1:]) + out.extend(merge_imports(ours, theirs, body)) + resolved += 1 + else: + out.extend(lines[i:end + 1]) + i = end + 1 + return "\n".join(out), resolved + + +def has_markers(text: str) -> bool: + return any(l.startswith(MARK_OURS) for l in text.split("\n")) + + +def move_files(changed: list[str], dirs: list[tuple[str, str]]) -> list[str]: + """Move files added under a source-only directory into the target's.""" + result = [] + for path in changed: + new = path + for src_dir, tgt_dir in dirs: + if path.startswith(src_dir + "/"): + candidate = tgt_dir + path[len(src_dir):] + if Path(candidate).exists(): + print(f"::warning::{path}: {candidate} already exists, left in place; move it by hand") + else: + Path(candidate).parent.mkdir(parents=True, exist_ok=True) + git("mv", path, candidate) + print(f"moved {path} -> {candidate}") + new = candidate + break + result.append(new) + return result + + +def gofmt(files: list[str]) -> None: + if not files: + return + if not shutil.which("gofmt"): + print("::warning::gofmt not on PATH; resolved import blocks are left unformatted") + return + res = subprocess.run(["gofmt", "-w", *files], capture_output=True, text=True) + if res.returncode != 0: + raise RuntimeError(f"gofmt rejected the result, nothing committed: {res.stderr.strip()}") + + +def run() -> dict[str, str]: + target = os.environ.get("TARGET_BRANCH", "") + branch = os.environ.get("BACKPORT_BRANCH", "") + source = os.environ.get("SOURCE_BRANCH") or "main" + remote = os.environ.get("REMOTE") or "origin" + push = os.environ.get("PUSH", "true") != "false" + if not target or not branch: + raise Skip("target-branch and backport-branch are required; pass both from the backport job") + + git("fetch", "-q", "--no-tags", remote, + *(f"+refs/heads/{b}:refs/remotes/{remote}/{b}" for b in (target, source, branch))) + gomods = [] + for b in (source, target): + content = git("show", f"{remote}/{b}:go.mod", check=False) + if not content: + raise Skip(f"no go.mod at the root of {b}; nothing to map") + gomods.append(content) + paths, dirs = module_mapping(*gomods) + if not paths and not dirs: + raise Skip(f"{source} and {target} use the same module paths") + + git("checkout", "-q", "-B", branch, f"{remote}/{branch}") + base = git("merge-base", f"{remote}/{target}", "HEAD").strip() + changed = [p for p in git("diff", "--name-only", "--diff-filter=AMR", base, "HEAD").splitlines() if p] + changed = move_files(changed, dirs) + moved = git("diff", "--cached", "--name-only").strip() != "" + + rewritten, to_format, hunks = [], [], 0 + for path in changed: + if not path.endswith(".go") or not Path(path).is_file(): + continue + before = Path(path).read_text(encoding="utf-8") + after, resolved = resolve_import_hunks(before, paths) + after = rewrite_paths(after, paths) + if after != before: + Path(path).write_text(after, encoding="utf-8") + rewritten.append(path) + hunks += resolved + if resolved and not has_markers(after): + to_format.append(path) + gofmt(to_format) + + remaining = [p for p in changed if Path(p).is_file() and has_markers(Path(p).read_text(encoding="utf-8", errors="replace"))] + for p in remaining: + print(f"::warning::{p}: conflict markers outside import blocks remain; resolve them by hand") + result = {"files-changed": str(len(rewritten)), "hunks-resolved": str(hunks), + "conflicts-remaining": str(len(remaining)), "commit": ""} + + if not rewritten and not moved: + return {**result, "conclusion": "unchanged", "reason": "no module paths to rewrite"} + + if rewritten: + git("add", "--", *rewritten) + mapping = ", ".join(f"{s} -> {t}" for s, t in paths) + git("-c", f"user.name={BOT_NAME}", "-c", f"user.email={BOT_EMAIL}", "commit", "-q", + "-m", f"fix(backport): use {target} module paths", "-m", f"Rewrote {mapping}.") + sha = git("rev-parse", "HEAD").strip() + result["commit"] = sha + if push: + git("push", "-q", remote, f"HEAD:refs/heads/{branch}") + reason = f"rewrote {len(rewritten)} file(s), resolved {hunks} import conflict(s)" + return {**result, "conclusion": "fixed", "reason": reason} + + +def main() -> int: + try: + result = run() + except Skip as e: + result = {"conclusion": "skipped", "reason": str(e)} + print(f"::notice::{e}") + except Exception as e: # noqa: BLE001 -- advisory: never fail the backport + result = {"conclusion": "failed", "reason": str(e).splitlines()[0] if str(e) else type(e).__name__} + print(f"::warning::backport-fix-imports failed: {result['reason']}; the backport PR is unchanged, fix it by hand") + for key, value in result.items(): + emit(key, value) + print(f"conclusion={result['conclusion']} reason={result['reason']}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/actions/backport-fix-imports/test/test_fix.py b/.github/actions/backport-fix-imports/test/test_fix.py new file mode 100644 index 00000000..d3acb092 --- /dev/null +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -0,0 +1,329 @@ +"""fix.py: the go.mod mapping, the import-hunk resolver, and full runs against +real throwaway git repos with a local bare remote. No network, no tokens.""" +from __future__ import annotations + +import importlib.util +import shutil +import subprocess +from pathlib import Path + +import pytest + +SRC = Path(__file__).resolve().parent.parent / "src" / "fix.py" + + +def load(): + spec = importlib.util.spec_from_file_location("backport_fix_imports", SRC) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +fix = load() + +V4_GOMOD = """module example.com/acme/app/v4 + +go 1.26 + +require example.com/acme/api/v4 v4.0.0 + +replace example.com/acme/api/v4 => ./staging/src/example.com/acme/api/v4 +""" +V5_GOMOD = V4_GOMOD.replace("/v4", "/v5") +API_V4 = "staging/src/example.com/acme/api/v4" +API_V5 = "staging/src/example.com/acme/api/v5" + + +def git(repo: Path, *args: str) -> str: + return subprocess.run(["git", "-C", str(repo), *args], check=True, + capture_output=True, text=True).stdout.strip() + + +def write(repo: Path, path: str, text: str) -> None: + (repo / path).parent.mkdir(parents=True, exist_ok=True) + (repo / path).write_text(text) + + +def commit_all(repo: Path, subject: str) -> None: + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", subject) + + +@pytest.fixture +def repo(tmp_path, monkeypatch): + """A clone with main on v5 and release-4.12 on v4, pushed to a bare remote. + Returns the clone; tests build the backport branch on top of release-4.12.""" + for key, value in {"GIT_AUTHOR_NAME": "test", "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "test", "GIT_COMMITTER_EMAIL": "test@example.com", + "GIT_CONFIG_GLOBAL": str(tmp_path / "gitconfig")}.items(): + monkeypatch.setenv(key, value) + monkeypatch.delenv("GH_TOKEN", raising=False) + remote = tmp_path / "remote.git" + subprocess.run(["git", "init", "-q", "--bare", str(remote)], check=True) + work = tmp_path / "work" + subprocess.run(["git", "init", "-q", "-b", "release-4.12", str(work)], check=True) + write(work, "go.mod", V4_GOMOD) + write(work, f"{API_V4}/types/types.go", "package types\n") + write(work, "pkg/peer/peer.go", 'package peer\n\nimport (\n\t"fmt"\n\n\t"example.com/acme/app/v4/pkg/util"\n)\n\nfunc A() { fmt.Println(util.X) }\n') + commit_all(work, "release line") + git(work, "checkout", "-q", "-b", "main") + write(work, "go.mod", V5_GOMOD) + git(work, "mv", API_V4, API_V5) + commit_all(work, "refactor: bump go modules to v5") + git(work, "remote", "add", "origin", str(remote)) + git(work, "push", "-q", "origin", "main", "release-4.12") + git(work, "checkout", "-q", "-b", "backport/release-4.12/pr-1", "release-4.12") + monkeypatch.chdir(work) + monkeypatch.setenv("TARGET_BRANCH", "release-4.12") + monkeypatch.setenv("BACKPORT_BRANCH", "backport/release-4.12/pr-1") + monkeypatch.setenv("GITHUB_OUTPUT", str(tmp_path / "output")) + return work + + +def push_backport(repo: Path) -> None: + commit_all(repo, "backport commit") + git(repo, "push", "-q", "origin", "backport/release-4.12/pr-1") + + +def outputs(repo: Path) -> dict[str, str]: + lines = (repo.parent / "output").read_text().splitlines() + return dict(l.split("=", 1) for l in lines) + + +def remote_file(repo: Path, path: str) -> str: + git(repo, "fetch", "-q", "origin") + return git(repo, "show", f"origin/backport/release-4.12/pr-1:{path}") + + +# -- mapping --------------------------------------------------------------- + +def test_mapping_covers_module_line_and_local_replaces(): + paths, dirs = fix.module_mapping(V5_GOMOD, V4_GOMOD) + assert ("example.com/acme/app/v5", "example.com/acme/app/v4") in paths + assert ("example.com/acme/api/v5", "example.com/acme/api/v4") in paths + assert dirs == [(API_V5, API_V4)] + + +def test_mapping_ignores_third_party_majors(): + src = V5_GOMOD + "require example.com/other/lib/v3 v3.0.0\n" + tgt = V4_GOMOD + "require example.com/other/lib/v2 v2.0.0\n" + paths, _ = fix.module_mapping(src, tgt) + assert all("other" not in s for s, _ in paths) + + +def test_mapping_reads_replace_blocks(): + gomod = "module example.com/acme/app/v5\n\nreplace (\n\texample.com/acme/api/v5 => ./staging/api/v5\n\texample.com/x => example.com/y v1.0.0\n)\n" + owned = fix.owned_modules(gomod) + assert owned["example.com/acme/api"] == ("example.com/acme/api/v5", "staging/api/v5") + assert "example.com/x" not in owned + + +def test_same_module_paths_map_to_nothing(): + assert fix.module_mapping(V4_GOMOD, V4_GOMOD) == ([], []) + + +# -- rewriting ------------------------------------------------------------- + +PATHS = [("example.com/acme/app/v5", "example.com/acme/app/v4")] + + +@pytest.mark.parametrize("text, expected", [ + ('"example.com/acme/app/v5/pkg/a"', '"example.com/acme/app/v4/pkg/a"'), + ('"example.com/acme/app/v5"', '"example.com/acme/app/v4"'), + ('"example.com/acme/app/v50/pkg"', '"example.com/acme/app/v50/pkg"'), + ("// moved from example.com/acme/app/v5/pkg", "// moved from example.com/acme/app/v5/pkg"), +]) +def test_rewrite_only_at_string_start_and_path_boundary(text, expected): + assert fix.rewrite_paths(text, PATHS) == expected + + +@pytest.mark.parametrize("line, name", [ + ('\t"example.com/acme/app/v4/pkg/peer"', "peer"), + ('\tstoragev1 "example.com/acme/api/v4/pkg/apis/storage/v1"', "storagev1"), + ('\t"example.com/acme/api/v4"', "api"), + ('\t"gopkg.in/yaml.v3"', "yaml"), + ('\t"github.com/go-openapi/spec"', "spec"), + ('\t"github.com/x/go-openapi"', None), + ('\t_ "embed"', None), + ('\t. "example.com/dot"', None), +]) +def test_import_name(line, name): + assert fix.import_name(line) == name + + +# -- resolving import hunks ------------------------------------------------ + +def conflicted(ours: list[str], theirs: list[str], body: str = "func F() {}\n") -> str: + return "\n".join(["package p", "", "import (", '\t"fmt"', "", + "<<<<<<< HEAD", *ours, "=======", *theirs, ">>>>>>> abc123 (feat: x (#1))", + ")", "", body]) + + +def test_import_only_hunk_takes_the_backported_side_rewritten(): + text = conflicted( + ['\t"example.com/acme/app/v4/pkg/apply"'], + ['\t"example.com/acme/app/v5/pkg/apply"', '\t"example.com/acme/app/v5/pkg/backup"'], + "func F() { apply.X(); backup.Y() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 1 + assert not fix.has_markers(out) + assert '"example.com/acme/app/v4/pkg/backup"' in out + assert out.count('"example.com/acme/app/v4/pkg/apply"') == 1 + assert "v5" not in out + + +def test_target_only_import_kept_while_used_dropped_when_not(): + text = conflicted( + ['\t"example.com/acme/app/v4/pkg/legacy"', '\t"example.com/acme/app/v4/pkg/gone"'], + ['\t"example.com/acme/app/v5/pkg/apply"'], + "func F() { legacy.Run(); apply.X() }\n") + out, _ = fix.resolve_import_hunks(text, PATHS) + assert '"example.com/acme/app/v4/pkg/legacy"' in out + assert "pkg/gone" not in out + + +def test_target_only_import_with_unknown_name_is_kept(): + text = conflicted(['\t"github.com/x/go-openapi"'], ['\t"example.com/acme/app/v5/pkg/apply"']) + out, _ = fix.resolve_import_hunks(text, PATHS) + assert '"github.com/x/go-openapi"' in out + + +def test_diff3_base_section_is_ignored(): + text = conflicted( + ['\t"example.com/acme/app/v4/pkg/a"', "||||||| base", '\t"example.com/acme/app/v4/pkg/old"'], + ['\t"example.com/acme/app/v5/pkg/a"'], "func F() { a.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 1 + assert "pkg/old" not in out and "|||||||" not in out + + +def test_code_hunk_is_left_alone(): + text = "\n".join(["package p", "", "func F() {", "<<<<<<< HEAD", "\ta()", "=======", "\tb()", + ">>>>>>> abc123", "}", ""]) + out, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 0 and out == text + + +def test_import_lines_outside_an_import_block_are_left_alone(): + text = "\n".join(["package p", "", "var s = []string{", "<<<<<<< HEAD", '\t"a"', "=======", '\t"b"', + ">>>>>>> abc123", "}", ""]) + _, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 0 + + +def test_unterminated_hunk_is_left_alone(): + text = "package p\n<<<<<<< HEAD\n\"a\"\n" + assert fix.resolve_import_hunks(text, PATHS) == (text, 0) + + +# -- full runs ------------------------------------------------------------- + +def test_clean_pick_with_source_imports_is_rewritten_and_pushed(repo): + write(repo, "pkg/peer/peer_test.go", 'package peer\n\nimport (\n\t"testing"\n\n\tstoragev1 "example.com/acme/api/v5/pkg/apis/storage/v1"\n\t"example.com/acme/app/v5/pkg/peer/networkpeer"\n)\n\nfunc TestX(t *testing.T) { _ = storagev1.X; networkpeer.Y() }\n') + push_backport(repo) + assert fix.main() == 0 + out = outputs(repo) + assert out["conclusion"] == "fixed" + assert out["files-changed"] == "1" + content = remote_file(repo, "pkg/peer/peer_test.go") + assert '"example.com/acme/api/v4/pkg/apis/storage/v1"' in content + assert '"example.com/acme/app/v4/pkg/peer/networkpeer"' in content + assert git(repo, "log", "-1", "--format=%s", "origin/backport/release-4.12/pr-1") == "fix(backport): use release-4.12 module paths" + + +def test_untouched_files_are_not_rewritten(repo): + # A v5 path in a file the backport did not change is not ours to touch. + write(repo, "pkg/other/other.go", 'package other\n\nimport _ "example.com/acme/app/v5/pkg/x"\n') + commit_all(repo, "pre-existing") + git(repo, "push", "-q", "origin", "HEAD:release-4.12") + write(repo, "pkg/peer/new.go", 'package peer\n\nimport "example.com/acme/app/v5/pkg/util"\n\nvar _ = util.X\n') + push_backport(repo) + fix.main() + assert '"example.com/acme/app/v5/pkg/x"' in remote_file(repo, "pkg/other/other.go") + assert '"example.com/acme/app/v4/pkg/util"' in remote_file(repo, "pkg/peer/new.go") + + +def test_file_added_under_source_staging_dir_is_moved(repo): + write(repo, f"{API_V5}/types/extra.go", 'package types\n\nimport "example.com/acme/api/v5/pkg/x"\n\nvar _ = x.Y\n') + push_backport(repo) + fix.main() + assert outputs(repo)["conclusion"] == "fixed" + tree = git(repo, "ls-tree", "-r", "--name-only", "origin/backport/release-4.12/pr-1") + assert f"{API_V4}/types/extra.go" in tree + assert API_V5 not in tree + assert '"example.com/acme/api/v4/pkg/x"' in remote_file(repo, f"{API_V4}/types/extra.go") + + +def test_import_conflict_resolved_code_conflict_counted(repo): + write(repo, "pkg/peer/peer.go", conflicted( + ['\t"example.com/acme/app/v4/pkg/util"'], + ['\t"example.com/acme/app/v5/pkg/util"', '\t"example.com/acme/app/v5/pkg/backup"'], + "func A() { fmt.Println(util.X); backup.Y() }\n").replace("package p", "package peer")) + write(repo, "pkg/peer/code.go", "package peer\n\nfunc B() {\n<<<<<<< HEAD\n\ta()\n=======\n\tb()\n>>>>>>> abc123\n}\n") + push_backport(repo) + fix.main() + out = outputs(repo) + assert out["conclusion"] == "fixed" + assert out["hunks-resolved"] == "1" + assert out["conflicts-remaining"] == "1" + assert not fix.has_markers(remote_file(repo, "pkg/peer/peer.go")) + assert fix.has_markers(remote_file(repo, "pkg/peer/code.go")) + + +@pytest.mark.skipif(not shutil.which("gofmt"), reason="gofmt not installed") +def test_resolved_import_block_is_gofmt_sorted(repo): + write(repo, "pkg/peer/peer.go", conflicted( + ['\t"example.com/acme/app/v4/pkg/util"'], + ['\t"example.com/acme/app/v5/pkg/zeta"', '\t"example.com/acme/app/v5/pkg/alpha"'], + "func A() { fmt.Println(util.X, zeta.X, alpha.X) }\n").replace("package p", "package peer")) + push_backport(repo) + fix.main() + content = remote_file(repo, "pkg/peer/peer.go") + assert content.index("pkg/alpha") < content.index("pkg/util") < content.index("pkg/zeta") + + +def test_second_run_is_a_no_op(repo): + write(repo, "pkg/peer/new.go", 'package peer\n\nimport "example.com/acme/app/v5/pkg/util"\n\nvar _ = util.X\n') + push_backport(repo) + fix.main() + head = git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") + (repo.parent / "output").unlink() + fix.main() + assert outputs(repo)["conclusion"] == "unchanged" + git(repo, "fetch", "-q", "origin") + assert git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") == head + + +def test_same_module_paths_skip(repo, monkeypatch): + git(repo, "push", "-q", "origin", "release-4.12:release-4.13") + monkeypatch.setenv("SOURCE_BRANCH", "release-4.13") + write(repo, "pkg/peer/new.go", "package peer\n") + push_backport(repo) + fix.main() + assert outputs(repo)["conclusion"] == "skipped" + + +def test_push_false_leaves_the_remote_alone(repo, monkeypatch): + monkeypatch.setenv("PUSH", "false") + write(repo, "pkg/peer/new.go", 'package peer\n\nimport "example.com/acme/app/v5/pkg/util"\n\nvar _ = util.X\n') + push_backport(repo) + before = git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") + fix.main() + out = outputs(repo) + assert out["conclusion"] == "fixed" and out["commit"] != before + git(repo, "fetch", "-q", "origin") + assert git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") == before + + +def test_missing_branch_fails_soft(repo, monkeypatch): + monkeypatch.setenv("BACKPORT_BRANCH", "backport/release-4.12/pr-404") + assert fix.main() == 0 + out = outputs(repo) + assert out["conclusion"] == "failed" + assert "git fetch failed" in out["reason"] + + +def test_missing_inputs_skip(repo, monkeypatch): + monkeypatch.delenv("TARGET_BRANCH") + assert fix.main() == 0 + assert outputs(repo)["conclusion"] == "skipped" diff --git a/.github/workflows/test-backport-fix-imports.yaml b/.github/workflows/test-backport-fix-imports.yaml new file mode 100644 index 00000000..36654482 --- /dev/null +++ b/.github/workflows/test-backport-fix-imports.yaml @@ -0,0 +1,27 @@ +name: Test backport-fix-imports + +on: + push: + branches: [main] + paths: + - '.github/actions/backport-fix-imports/**' + - '.github/workflows/test-backport-fix-imports.yaml' + pull_request: + paths: + - '.github/actions/backport-fix-imports/**' + - '.github/workflows/test-backport-fix-imports.yaml' + +permissions: + contents: read + +jobs: + pytest: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Install uv + uses: astral-sh/setup-uv@c18668ad3cf93ea998bef934396af7bb5c839dc7 # v10.2.0 + - name: Run backport-fix-imports pytest + run: uv run --with pytest==8.3.3 pytest .github/actions/backport-fix-imports/test/ -q diff --git a/Makefile b/Makefile index 72b7ffed..ba15eca4 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: test test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-ci-test-notify test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-publish-helm-chart test-govulncheck test-go-licenses test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check build-linear-release-sync lint check-bats-jobs install-auto-doc generate-docs check-docs help +.PHONY: test test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-ci-test-notify test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-publish-helm-chart test-govulncheck test-go-licenses test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-backport-fix-imports test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check build-linear-release-sync lint check-bats-jobs install-auto-doc generate-docs check-docs help ACTIONS_DIR := .github/actions WORKFLOWS_DIR := .github/workflows @@ -93,7 +93,7 @@ check-docs: generate-docs ## verify docs are up to date (fails if drift detected help: ## show this help @grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf " %-30s %s\n", $$1, $$2}' -test: test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-ci-test-notify test-go-licenses test-publish-helm-chart test-govulncheck test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check ## run all action tests +test: test-secret-broker-request test-secret-broker-response test-semver-validation test-setup-semstat test-linear-pr-commenter test-link-backport-prs test-select-backport-source-prs test-release-notification test-linear-release-sync test-linear-cve-summary test-aws-test-infra test-cleanup-head-charts test-auto-approve-bot-prs test-ai-pr-review test-ai-step test-release-summary test-ci-test-notify test-go-licenses test-publish-helm-chart test-govulncheck test-run-ginkgo test-sticky-pr-comment test-repository-dispatch test-parse-label-filter test-release-branch-freeze test-prerelease-setup test-vcluster-release test-platform-release test-release-lib test-subtree-mirror test-oss-commit-sync test-oss-mirror-staleness test-backport-legacy-allowlist test-backport-legacy-split test-backport-fix-imports test-promote-release test-wait-for-release test-cve-scan test-commitlint test-resolve-github-release test-comment-triggered-check ## run all action tests test-secret-broker-request: ## run secret-broker-request pytest tests uv run --with pytest==8.3.3 pytest $(ACTIONS_DIR)/secret-broker-request/test/ -q @@ -211,6 +211,9 @@ test-backport-legacy-allowlist: ## run backport-legacy-allowlist bats tests test-backport-legacy-split: ## run backport-legacy-split bats tests bats $(ACTIONS_DIR)/backport-legacy-split/test/run.bats +test-backport-fix-imports: ## run backport-fix-imports pytest tests + uv run --with pytest==8.3.3 pytest $(ACTIONS_DIR)/backport-fix-imports/test/ -q + test-wait-for-release: ## run wait-for-release bats tests bats $(ACTIONS_DIR)/wait-for-release/test/*.bats diff --git a/README.md b/README.md index dd7f27da..693e2aab 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,14 @@ backport link sweep and performs no mutation. See [select-backport-source-prs README](./.github/actions/select-backport-source-prs/README.md) for inputs, outputs, and tests. +### Backport Fix Imports Action + +Repairs Go module paths on a backport branch whose target release line is on an older module major than its source, for example a v5 `main` backported to a v4 release branch. Rewrites the import paths of modules the repository owns, moves files added under a source-only staging directory, resolves conflict hunks made only of import lines, and pushes one fix commit. Conflicts in real code are left and counted; advisory, never fails the job. + +**Location:** `.github/actions/backport-fix-imports` + +See [backport-fix-imports README](./.github/actions/backport-fix-imports/README.md) for inputs, outputs, and tests. + ### Run Ginkgo Tests Runs Ginkgo tests with directory or label-based filtering and generates a From 5131bd21bd024b0599a3b904214b1d16868f37a6 Mon Sep 17 00:00:00 2001 From: Caue Santos Date: Thu, 22 Oct 2026 11:17:34 -0600 Subject: [PATCH 2/7] fix(backport-fix-imports): keep versioned package imports and report failed when the script cannot start --- .../actions/backport-fix-imports/action.yml | 6 ++-- .../actions/backport-fix-imports/src/fix.py | 10 +++--- .../backport-fix-imports/test/test_fix.py | 13 +++++++- .../workflows/test-backport-fix-imports.yaml | 31 +++++++++++++++++++ 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/.github/actions/backport-fix-imports/action.yml b/.github/actions/backport-fix-imports/action.yml index e513b83e..e6c517ce 100644 --- a/.github/actions/backport-fix-imports/action.yml +++ b/.github/actions/backport-fix-imports/action.yml @@ -29,10 +29,10 @@ inputs: outputs: conclusion: description: 'fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input) or failed.' - value: ${{ steps.fix.outputs.conclusion }} + value: ${{ steps.fix.outputs.conclusion || 'failed' }} reason: description: 'One-line explanation of the conclusion.' - value: ${{ steps.fix.outputs.reason }} + value: ${{ steps.fix.outputs.reason || 'the fix step did not run; see its log' }} files-changed: description: 'Number of .go files rewritten.' value: ${{ steps.fix.outputs.files-changed }} @@ -51,6 +51,8 @@ runs: steps: # The script exits 0 on every path it controls; continue-on-error covers # the rest (a missing interpreter), so a backport is never failed by it. + # When the script never ran it wrote no outputs, so conclusion and reason + # fall back to failed above. - name: Fix module paths id: fix continue-on-error: true diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index da20bcbf..df2e5a78 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -131,10 +131,12 @@ def import_name(line: str) -> str | None: return None if m.group(1): return m.group(1) - parts = m.group(2).split("/") - last = parts[-1] - if re.fullmatch(r"v\d+", last) and len(parts) > 1: - last = parts[-2] + last = m.group(2).split("/")[-1] + # A /vN element is either a module major (package named after its parent) + # or the package itself (k8s.io/api/core/v1 is package v1); the path + # cannot tell which. + if re.fullmatch(r"v\d+", last): + return None last = re.sub(r"\.v\d+$", "", last) return last if IDENT.fullmatch(last) else None diff --git a/.github/actions/backport-fix-imports/test/test_fix.py b/.github/actions/backport-fix-imports/test/test_fix.py index d3acb092..5d15f6c9 100644 --- a/.github/actions/backport-fix-imports/test/test_fix.py +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -140,7 +140,8 @@ def test_rewrite_only_at_string_start_and_path_boundary(text, expected): @pytest.mark.parametrize("line, name", [ ('\t"example.com/acme/app/v4/pkg/peer"', "peer"), ('\tstoragev1 "example.com/acme/api/v4/pkg/apis/storage/v1"', "storagev1"), - ('\t"example.com/acme/api/v4"', "api"), + ('\t"example.com/acme/api/v4"', None), + ('\t"k8s.io/api/core/v1"', None), ('\t"gopkg.in/yaml.v3"', "yaml"), ('\t"github.com/go-openapi/spec"', "spec"), ('\t"github.com/x/go-openapi"', None), @@ -188,6 +189,16 @@ def test_target_only_import_with_unknown_name_is_kept(): assert '"github.com/x/go-openapi"' in out +def test_target_only_versioned_package_is_kept(): + # k8s.io/api/core/v1 is package v1, not core: guessing "core" would drop + # an import the body still uses. + text = conflicted(['\t"k8s.io/api/core/v1"'], ['\t"example.com/acme/app/v5/pkg/apply"'], + "func F() { _ = v1.Pod{}; apply.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 1 + assert '"k8s.io/api/core/v1"' in out + + def test_diff3_base_section_is_ignored(): text = conflicted( ['\t"example.com/acme/app/v4/pkg/a"', "||||||| base", '\t"example.com/acme/app/v4/pkg/old"'], diff --git a/.github/workflows/test-backport-fix-imports.yaml b/.github/workflows/test-backport-fix-imports.yaml index 36654482..47acec7f 100644 --- a/.github/workflows/test-backport-fix-imports.yaml +++ b/.github/workflows/test-backport-fix-imports.yaml @@ -25,3 +25,34 @@ jobs: uses: astral-sh/setup-uv@c18668ad3cf93ea998bef934396af7bb5c839dc7 # v10.2.0 - name: Run backport-fix-imports pytest run: uv run --with pytest==8.3.3 pytest .github/actions/backport-fix-imports/test/ -q + + # The composite's own contract: when the script cannot start it writes no + # outputs, and the action must still report conclusion=failed and stay green. + smoke-no-interpreter: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Shadow python3 with one that cannot start + run: | + mkdir -p "$RUNNER_TEMP/broken" + printf '#!/bin/sh\nexit 127\n' > "$RUNNER_TEMP/broken/python3" + chmod +x "$RUNNER_TEMP/broken/python3" + echo "$RUNNER_TEMP/broken" >> "$GITHUB_PATH" + - name: Run backport-fix-imports + id: fix + uses: ./.github/actions/backport-fix-imports + with: + target-branch: release-0.0 + backport-branch: backport/release-0.0/pr-0 + github-token: ${{ github.token }} + push: 'false' + - name: Verify it reported failed + env: + CONCLUSION: ${{ steps.fix.outputs.conclusion }} + REASON: ${{ steps.fix.outputs.reason }} + run: | + echo "conclusion=$CONCLUSION reason=$REASON" + [ "$CONCLUSION" = failed ] + [ -n "$REASON" ] From 0173056b80ab8d1c5c0d169850fab226127852d8 Mon Sep 17 00:00:00 2001 From: Caue Santos Date: Thu, 22 Oct 2026 11:32:02 -0600 Subject: [PATCH 3/7] fix(backport-fix-imports): address panel review on import usage, outputs and grouping --- .../actions/backport-fix-imports/README.md | 20 +++++++------- .../actions/backport-fix-imports/action.yml | 8 +++--- .../actions/backport-fix-imports/src/fix.py | 15 +++++++---- .../backport-fix-imports/test/test_fix.py | 26 ++++++++++++++++++- 4 files changed, 50 insertions(+), 19 deletions(-) diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md index 42bc7cd4..5a04fd55 100644 --- a/.github/actions/backport-fix-imports/README.md +++ b/.github/actions/backport-fix-imports/README.md @@ -50,7 +50,9 @@ a one-line `reason` and exit 0, and nothing is pushed. backport-branch: backport/release-4.12/pr-8606 github-token: ${{ secrets.GH_ACCESS_TOKEN }} -- if: steps.fix.outputs.conflicts-remaining != '0' +- if: >- + (steps.fix.outputs.conclusion == 'fixed' || steps.fix.outputs.conclusion == 'unchanged') && + steps.fix.outputs.conflicts-remaining != '0' run: echo "conflicts left for review" ``` @@ -73,14 +75,14 @@ the backport PR's CI. -| OUTPUT | TYPE | DESCRIPTION | -|---------------------|--------|-------------------------------------------------------------------------------------------------------------------------------------| -| commit | string | SHA of the fix commit; empty
when none was made. | -| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | -| conflicts-remaining | string | Number of changed files that still
carry conflict markers after the fix. | -| files-changed | string | Number of .go files rewritten. | -| hunks-resolved | string | Number of import-only conflict hunks resolved. | -| reason | string | One-line explanation of the conclusion. | +| OUTPUT | TYPE | DESCRIPTION | +|---------------------|--------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| commit | string | SHA of the fix commit; empty
when none was made. | +| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | +| conflicts-remaining | string | Number of changed files that still
carry conflict markers after the fix.
0 on skipped and failed, where
nothing was checked, so gate on
conclusion too. | +| files-changed | string | Number of .go files rewritten. | +| hunks-resolved | string | Number of import-only conflict hunks resolved. | +| reason | string | One-line explanation of the conclusion. | diff --git a/.github/actions/backport-fix-imports/action.yml b/.github/actions/backport-fix-imports/action.yml index e6c517ce..fc98057f 100644 --- a/.github/actions/backport-fix-imports/action.yml +++ b/.github/actions/backport-fix-imports/action.yml @@ -35,13 +35,13 @@ outputs: value: ${{ steps.fix.outputs.reason || 'the fix step did not run; see its log' }} files-changed: description: 'Number of .go files rewritten.' - value: ${{ steps.fix.outputs.files-changed }} + value: ${{ steps.fix.outputs.files-changed || '0' }} hunks-resolved: description: 'Number of import-only conflict hunks resolved.' - value: ${{ steps.fix.outputs.hunks-resolved }} + value: ${{ steps.fix.outputs.hunks-resolved || '0' }} conflicts-remaining: - description: 'Number of changed files that still carry conflict markers after the fix.' - value: ${{ steps.fix.outputs.conflicts-remaining }} + description: 'Number of changed files that still carry conflict markers after the fix. 0 on skipped and failed, where nothing was checked, so gate on conclusion too.' + value: ${{ steps.fix.outputs.conflicts-remaining || '0' }} commit: description: 'SHA of the fix commit; empty when none was made.' value: ${{ steps.fix.outputs.commit }} diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index df2e5a78..21f7b071 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -78,7 +78,8 @@ def git(*args: str, check: bool = True) -> str: GIT_CONFIG_VALUE_0=f"AUTHORIZATION: basic {auth}") res = subprocess.run(["git", *args], capture_output=True, text=True, env=env) if check and res.returncode != 0: - raise RuntimeError(f"git {args[0]} failed: {res.stderr.strip()}") + sub = next(a for i, a in enumerate(args) if not a.startswith("-") and (i == 0 or args[i - 1] != "-c")) + raise RuntimeError(f"git {sub} failed: {res.stderr.strip()}") return res.stdout @@ -173,10 +174,12 @@ def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str]: if not m or m.group(2) in seen: continue name = import_name(line) - if name is None or re.search(r"\b" + re.escape(name) + r"\.", body): + # Not after a dot: obj.config.Field is a field, not package config. + if name is None or re.search(r"(? tuple[str, int]: @@ -289,7 +292,7 @@ def run() -> dict[str, str]: remaining = [p for p in changed if Path(p).is_file() and has_markers(Path(p).read_text(encoding="utf-8", errors="replace"))] for p in remaining: - print(f"::warning::{p}: conflict markers outside import blocks remain; resolve them by hand") + print(f"::warning::{p}: conflict markers remain; resolve them by hand") result = {"files-changed": str(len(rewritten)), "hunks-resolved": str(hunks), "conflicts-remaining": str(len(remaining)), "commit": ""} @@ -318,7 +321,9 @@ def main() -> int: except Exception as e: # noqa: BLE001 -- advisory: never fail the backport result = {"conclusion": "failed", "reason": str(e).splitlines()[0] if str(e) else type(e).__name__} print(f"::warning::backport-fix-imports failed: {result['reason']}; the backport PR is unchanged, fix it by hand") - for key, value in result.items(): + # Every output on every path: an empty count would read as "not 0". + defaults = {"files-changed": "0", "hunks-resolved": "0", "conflicts-remaining": "0", "commit": ""} + for key, value in {**defaults, **result}.items(): emit(key, value) print(f"conclusion={result['conclusion']} reason={result['reason']}") return 0 diff --git a/.github/actions/backport-fix-imports/test/test_fix.py b/.github/actions/backport-fix-imports/test/test_fix.py index 5d15f6c9..5b51e343 100644 --- a/.github/actions/backport-fix-imports/test/test_fix.py +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -183,6 +183,27 @@ def test_target_only_import_kept_while_used_dropped_when_not(): assert "pkg/gone" not in out +def test_field_access_is_not_a_package_use(): + text = conflicted(['\t"example.com/acme/app/v4/pkg/config"'], ['\t"example.com/acme/app/v5/pkg/apply"'], + "func F() { x := obj.config.Field; apply.X(x) }\n") + out, _ = fix.resolve_import_hunks(text, PATHS) + assert "pkg/config" not in out + + +@pytest.mark.parametrize("use", ["config.Load()", "(config.X)", "*config.T", "[]config.T{}"]) +def test_package_use_forms_keep_the_import(use): + text = conflicted(['\t"example.com/acme/app/v4/pkg/config"'], ['\t"example.com/acme/app/v5/pkg/apply"'], + f"func F() {{ _ = {use}; apply.X() }}\n") + out, _ = fix.resolve_import_hunks(text, PATHS) + assert '"example.com/acme/app/v4/pkg/config"' in out + + +def test_blank_lines_between_import_groups_are_kept(): + theirs = ['\t"strings"', "", '\t"example.com/acme/app/v5/pkg/apply"'] + out, _ = fix.resolve_import_hunks(conflicted([], theirs, "func F() { strings.X(); apply.X() }\n"), PATHS) + assert '\t"strings"\n\n\t"example.com/acme/app/v4/pkg/apply"' in out + + def test_target_only_import_with_unknown_name_is_kept(): text = conflicted(['\t"github.com/x/go-openapi"'], ['\t"example.com/acme/app/v5/pkg/apply"']) out, _ = fix.resolve_import_hunks(text, PATHS) @@ -332,9 +353,12 @@ def test_missing_branch_fails_soft(repo, monkeypatch): out = outputs(repo) assert out["conclusion"] == "failed" assert "git fetch failed" in out["reason"] + assert out["conflicts-remaining"] == "0" and out["commit"] == "" def test_missing_inputs_skip(repo, monkeypatch): monkeypatch.delenv("TARGET_BRANCH") assert fix.main() == 0 - assert outputs(repo)["conclusion"] == "skipped" + out = outputs(repo) + assert out["conclusion"] == "skipped" + assert out["files-changed"] == out["hunks-resolved"] == out["conflicts-remaining"] == "0" From c91f4fd87600fffbde9ee65d1e97ebee03e52492 Mon Sep 17 00:00:00 2001 From: Caue Santos Date: Thu, 22 Oct 2026 11:51:16 -0600 Subject: [PATCH 4/7] fix(backport-fix-imports): leave undecidable import hunks and blocked moves for review --- .../actions/backport-fix-imports/README.md | 27 ++++---- .../actions/backport-fix-imports/action.yml | 2 +- .../actions/backport-fix-imports/src/fix.py | 56 +++++++++++------ .../backport-fix-imports/test/test_fix.py | 63 ++++++++++++++++--- 4 files changed, 107 insertions(+), 41 deletions(-) diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md index 5a04fd55..cea9cb49 100644 --- a/.github/actions/backport-fix-imports/README.md +++ b/.github/actions/backport-fix-imports/README.md @@ -18,18 +18,21 @@ already exists on the remote: rewritten, so a dependency major bump on `main` is left alone. 2. Moves files the backport added under a source-only local directory (for example `staging/src/github.com/loft-sh/api/v5/...`) into the target's - directory. + directory. A file whose target path already exists is not moved and is + left for review. 3. Resolves conflict hunks made only of import lines inside an import block. The backported side wins, rewritten to the target paths. An import only the - target side had stays while the file still uses it. + target side had stays while the code uses it, ignoring comments and strings. + When its package name cannot be told from the path (a `/vN` path such as + `k8s.io/api/core/v1`), the hunk is left for review instead of guessed. 4. Rewrites the mapped paths in the `.go` files the backport changed, only at the start of a string literal and on a path boundary. 5. Runs `gofmt` on files whose import blocks it resolved, then commits as `github-actions[bot]` and pushes one commit: `fix(backport): use module paths`. -Conflict hunks in real code are left as they are and counted in -`conflicts-remaining`, with a warning per file. A second run finds nothing to +Conflict hunks in real code, undecidable import hunks and blocked moves are +left as they are and counted in `conflicts-remaining`, with a warning per file. A second run finds nothing to rewrite and reports `unchanged`, so re-runs are safe. The action never fails the job: every failure path ends with `conclusion=failed`, @@ -75,14 +78,14 @@ the backport PR's CI. -| OUTPUT | TYPE | DESCRIPTION | -|---------------------|--------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| commit | string | SHA of the fix commit; empty
when none was made. | -| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | -| conflicts-remaining | string | Number of changed files that still
carry conflict markers after the fix.
0 on skipped and failed, where
nothing was checked, so gate on
conclusion too. | -| files-changed | string | Number of .go files rewritten. | -| hunks-resolved | string | Number of import-only conflict hunks resolved. | -| reason | string | One-line explanation of the conclusion. | +| OUTPUT | TYPE | DESCRIPTION | +|---------------------|--------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| commit | string | SHA of the fix commit; empty
when none was made. | +| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | +| conflicts-remaining | string | Number of changed files left for
a person: conflict markers remain, or
a new file could not be
moved because its target path exists.
0 on skipped and failed, where
nothing was checked, so gate on
conclusion too. | +| files-changed | string | Number of .go files rewritten. | +| hunks-resolved | string | Number of import-only conflict hunks resolved. | +| reason | string | One-line explanation of the conclusion. | diff --git a/.github/actions/backport-fix-imports/action.yml b/.github/actions/backport-fix-imports/action.yml index fc98057f..6f51920b 100644 --- a/.github/actions/backport-fix-imports/action.yml +++ b/.github/actions/backport-fix-imports/action.yml @@ -40,7 +40,7 @@ outputs: description: 'Number of import-only conflict hunks resolved.' value: ${{ steps.fix.outputs.hunks-resolved || '0' }} conflicts-remaining: - description: 'Number of changed files that still carry conflict markers after the fix. 0 on skipped and failed, where nothing was checked, so gate on conclusion too.' + description: 'Number of changed files left for a person: conflict markers remain, or a new file could not be moved because its target path exists. 0 on skipped and failed, where nothing was checked, so gate on conclusion too.' value: ${{ steps.fix.outputs.conflicts-remaining || '0' }} commit: description: 'SHA of the fix commit; empty when none was made.' diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index 21f7b071..bff1d1e6 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -54,6 +54,8 @@ IMPORT_SPEC = re.compile(r'^\s*(?:([A-Za-z_][A-Za-z0-9_]*|\.)\s+)?"([^"\s]+)"\s*(?://.*)?$') COMMENT = re.compile(r"^\s*//") IDENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +# Comments and string, raw string and rune literals: a name inside them is not a use. +GO_NOISE = re.compile(r"""//[^\n]*|/\*.*?\*/|"(?:\\.|[^"\\\n])*"|`[^`]*`|'(?:\\.|[^'\\\n])*'""", re.S) BOT_NAME = "github-actions[bot]" BOT_EMAIL = "41898282+github-actions[bot]@users.noreply.github.com" @@ -163,19 +165,23 @@ def block_end(lines: list[str], index: int) -> int: return len(lines) -def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str]: +def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str] | None: """Theirs (the backported change) wins. An import only the target side - has stays while the file still uses it, and whenever its name cannot be - told: an unused import fails the build as surely as a missing one.""" + has stays while the code uses it. Returns None when that cannot be told + from the path, so the hunk is left for review: an unused import fails the + build as surely as a missing one.""" merged = list(theirs) seen = {m.group(2) for m in map(IMPORT_SPEC.match, theirs) if m} + code = GO_NOISE.sub(" ", body) for line in ours: m = IMPORT_SPEC.match(line) if not m or m.group(2) in seen: continue name = import_name(line) + if m.group(1) != "_" and name is None: + return None # Not after a dot: obj.config.Field is a field, not package config. - if name is None or re.search(r"(? tuple[str, base = next((j for j in range(i + 1, sep) if lines[j].startswith(MARK_BASE)), sep) ours = [rewrite_paths(l, paths) for l in lines[i + 1:base]] theirs = [rewrite_paths(l, paths) for l in lines[sep + 1:end]] + merged = None if is_import_only(ours) and is_import_only(theirs) and in_import_block(lines, i): - body = "\n".join(lines[block_end(lines, end + 1) + 1:]) - out.extend(merge_imports(ours, theirs, body)) + merged = merge_imports(ours, theirs, "\n".join(lines[block_end(lines, end + 1) + 1:])) + if merged is not None: + out.extend(merged) resolved += 1 else: out.extend(lines[i:end + 1]) @@ -217,16 +225,28 @@ def has_markers(text: str) -> bool: return any(l.startswith(MARK_OURS) for l in text.split("\n")) -def move_files(changed: list[str], dirs: list[tuple[str, str]]) -> list[str]: - """Move files added under a source-only directory into the target's.""" - result = [] - for path in changed: +def changed_files(base: str) -> list[tuple[str, str]]: + """(status letter, current path) for each file the backport added, modified or renamed.""" + files = [] + for line in git("diff", "--name-status", "--diff-filter=AMR", base, "HEAD").splitlines(): + fields = line.split("\t") + files.append((fields[0][0], fields[-1])) + return files + + +def move_files(changed: list[tuple[str, str]], dirs: list[tuple[str, str]]) -> tuple[list[str], list[str]]: + """Move files the backport added under a source-only directory into the + target's. Returns the resulting paths and the files that could not move + because their target path exists; those are left for review.""" + result, blocked = [], [] + for status, path in changed: new = path for src_dir, tgt_dir in dirs: - if path.startswith(src_dir + "/"): + if status == "A" and path.startswith(src_dir + "/"): candidate = tgt_dir + path[len(src_dir):] if Path(candidate).exists(): - print(f"::warning::{path}: {candidate} already exists, left in place; move it by hand") + print(f"::warning::{path}: {candidate} already exists, so it was not moved; merge the two by hand") + blocked.append(path) else: Path(candidate).parent.mkdir(parents=True, exist_ok=True) git("mv", path, candidate) @@ -234,7 +254,7 @@ def move_files(changed: list[str], dirs: list[tuple[str, str]]) -> list[str]: new = candidate break result.append(new) - return result + return result, blocked def gofmt(files: list[str]) -> None: @@ -271,13 +291,12 @@ def run() -> dict[str, str]: git("checkout", "-q", "-B", branch, f"{remote}/{branch}") base = git("merge-base", f"{remote}/{target}", "HEAD").strip() - changed = [p for p in git("diff", "--name-only", "--diff-filter=AMR", base, "HEAD").splitlines() if p] - changed = move_files(changed, dirs) + changed, blocked = move_files(changed_files(base), dirs) moved = git("diff", "--cached", "--name-only").strip() != "" rewritten, to_format, hunks = [], [], 0 for path in changed: - if not path.endswith(".go") or not Path(path).is_file(): + if path in blocked or not path.endswith(".go") or not Path(path).is_file(): continue before = Path(path).read_text(encoding="utf-8") after, resolved = resolve_import_hunks(before, paths) @@ -290,9 +309,10 @@ def run() -> dict[str, str]: to_format.append(path) gofmt(to_format) - remaining = [p for p in changed if Path(p).is_file() and has_markers(Path(p).read_text(encoding="utf-8", errors="replace"))] - for p in remaining: + marked = [p for p in changed if Path(p).is_file() and has_markers(Path(p).read_text(encoding="utf-8", errors="replace"))] + for p in marked: print(f"::warning::{p}: conflict markers remain; resolve them by hand") + remaining = sorted(set(marked) | set(blocked)) result = {"files-changed": str(len(rewritten)), "hunks-resolved": str(hunks), "conflicts-remaining": str(len(remaining)), "commit": ""} diff --git a/.github/actions/backport-fix-imports/test/test_fix.py b/.github/actions/backport-fix-imports/test/test_fix.py index 5b51e343..2d55afa3 100644 --- a/.github/actions/backport-fix-imports/test/test_fix.py +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -204,20 +204,39 @@ def test_blank_lines_between_import_groups_are_kept(): assert '\t"strings"\n\n\t"example.com/acme/app/v4/pkg/apply"' in out -def test_target_only_import_with_unknown_name_is_kept(): - text = conflicted(['\t"github.com/x/go-openapi"'], ['\t"example.com/acme/app/v5/pkg/apply"']) - out, _ = fix.resolve_import_hunks(text, PATHS) - assert '"github.com/x/go-openapi"' in out +@pytest.mark.parametrize("line", [ + '\t"github.com/x/go-openapi"', + # k8s.io/api/core/v1 is package v1, k8s.io/klog/v2 is package klog: the + # path alone cannot tell, so neither keeping nor dropping is safe. + '\t"k8s.io/api/core/v1"', + '\t"k8s.io/klog/v2"', + '\t. "example.com/dot"', +]) +def test_target_only_import_with_unknown_name_leaves_the_hunk(line): + text = conflicted([line], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { _ = v1.Pod{}; apply.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 0 + assert out == text + + +def test_target_only_blank_import_is_kept(): + text = conflicted(['\t_ "embed"'], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { apply.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS) + assert resolved == 1 + assert '_ "embed"' in out -def test_target_only_versioned_package_is_kept(): - # k8s.io/api/core/v1 is package v1, not core: guessing "core" would drop - # an import the body still uses. - text = conflicted(['\t"k8s.io/api/core/v1"'], ['\t"example.com/acme/app/v5/pkg/apply"'], - "func F() { _ = v1.Pod{}; apply.X() }\n") +@pytest.mark.parametrize("body", [ + "// config.Load() is called by the caller\nfunc F() { apply.X() }\n", + "/* config.Load() */\nfunc F() { apply.X() }\n", + 'var s = "config.Load()"\nfunc F() { apply.X() }\n', + "var s = `config.Load()`\nfunc F() { apply.X() }\n", +]) +def test_use_in_comment_or_string_does_not_keep_the_import(body): + text = conflicted(['\t"example.com/acme/app/v4/pkg/config"'], ['\t"example.com/acme/app/v5/pkg/apply"'], body) out, resolved = fix.resolve_import_hunks(text, PATHS) assert resolved == 1 - assert '"k8s.io/api/core/v1"' in out + assert "pkg/config" not in out def test_diff3_base_section_is_ignored(): @@ -275,6 +294,30 @@ def test_untouched_files_are_not_rewritten(repo): assert '"example.com/acme/app/v4/pkg/util"' in remote_file(repo, "pkg/peer/new.go") +def test_existing_file_under_source_dir_is_not_moved(repo): + # Only files the backport added move; one the target already had stays put. + write(repo, f"{API_V5}/types/kept.go", "package types\n") + commit_all(repo, "pre-existing") + git(repo, "push", "-q", "origin", "HEAD:release-4.12") + write(repo, f"{API_V5}/types/kept.go", "package types\n\nvar X = 1\n") + write(repo, "pkg/peer/new.go", 'package peer\n\nimport "example.com/acme/app/v5/pkg/util"\n\nvar _ = util.X\n') + push_backport(repo) + fix.main() + tree = git(repo, "ls-tree", "-r", "--name-only", "origin/backport/release-4.12/pr-1") + assert f"{API_V5}/types/kept.go" in tree + assert f"{API_V4}/types/kept.go" not in tree + + +def test_move_blocked_by_existing_target_is_left_for_review(repo): + write(repo, f"{API_V5}/types/types.go", 'package types\n\nimport "example.com/acme/api/v5/pkg/x"\n\nvar _ = x.Y\n') + push_backport(repo) + fix.main() + out = outputs(repo) + assert out["conflicts-remaining"] == "1" + assert out["conclusion"] == "unchanged" + assert '"example.com/acme/api/v5/pkg/x"' in remote_file(repo, f"{API_V5}/types/types.go") + + def test_file_added_under_source_staging_dir_is_moved(repo): write(repo, f"{API_V5}/types/extra.go", 'package types\n\nimport "example.com/acme/api/v5/pkg/x"\n\nvar _ = x.Y\n') push_backport(repo) From cb32a74de9fb938de5005c626c12201e0e3269c9 Mon Sep 17 00:00:00 2001 From: Caue Santos Date: Thu, 22 Oct 2026 12:39:46 -0600 Subject: [PATCH 5/7] fix(backport-fix-imports): leave import hunks for review when a path changes name --- .../actions/backport-fix-imports/README.md | 3 ++- .../actions/backport-fix-imports/src/fix.py | 12 ++++++++--- .../backport-fix-imports/test/test_fix.py | 21 +++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md index cea9cb49..ff1bc850 100644 --- a/.github/actions/backport-fix-imports/README.md +++ b/.github/actions/backport-fix-imports/README.md @@ -24,7 +24,8 @@ already exists on the remote: The backported side wins, rewritten to the target paths. An import only the target side had stays while the code uses it, ignoring comments and strings. When its package name cannot be told from the path (a `/vN` path such as - `k8s.io/api/core/v1`), the hunk is left for review instead of guessed. + `k8s.io/api/core/v1`), or when both sides import the same path under + different names, the hunk is left for review instead of guessed. 4. Rewrites the mapped paths in the `.go` files the backport changed, only at the start of a string literal and on a path boundary. 5. Runs `gofmt` on files whose import blocks it resolved, then commits as diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index bff1d1e6..4996900e 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -171,11 +171,17 @@ def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str] | from the path, so the hunk is left for review: an unused import fails the build as surely as a missing one.""" merged = list(theirs) - seen = {m.group(2) for m in map(IMPORT_SPEC.match, theirs) if m} + seen = {m.group(2): m.group(1) for m in map(IMPORT_SPEC.match, theirs) if m} code = GO_NOISE.sub(" ", body) for line in ours: m = IMPORT_SPEC.match(line) - if not m or m.group(2) in seen: + if not m: + continue + if m.group(2) in seen: + # Same path under another name (or blank/dot): the code outside + # the hunk may still use the target's name. + if seen[m.group(2)] != m.group(1): + return None continue name = import_name(line) if m.group(1) != "_" and name is None: @@ -183,7 +189,7 @@ def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str] | # Not after a dot: obj.config.Field is a field, not package config. if m.group(1) == "_" or re.search(r"(? Date: Thu, 22 Oct 2026 13:05:39 -0600 Subject: [PATCH 6/7] fix(backport-fix-imports): read package names from source and keep import comments --- .../actions/backport-fix-imports/README.md | 10 +- .../actions/backport-fix-imports/src/fix.py | 101 +++++++++++------ .../backport-fix-imports/test/test_fix.py | 106 ++++++++++++++---- 3 files changed, 158 insertions(+), 59 deletions(-) diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md index ff1bc850..3dca4977 100644 --- a/.github/actions/backport-fix-imports/README.md +++ b/.github/actions/backport-fix-imports/README.md @@ -23,9 +23,13 @@ already exists on the remote: 3. Resolves conflict hunks made only of import lines inside an import block. The backported side wins, rewritten to the target paths. An import only the target side had stays while the code uses it, ignoring comments and strings. - When its package name cannot be told from the path (a `/vN` path such as - `k8s.io/api/core/v1`), or when both sides import the same path under - different names, the hunk is left for review instead of guessed. + The package name is read from the source, never guessed from the path + (`github.com/json-iterator/go` is package `jsoniter`): the standard library + by its last element, the repository's own modules from their directory, and + everything else from `vendor/`. The hunk is left for review instead when + the name cannot be read, when both sides import the same path differently + (another name, another comment such as `//nolint`), or when a comment line + only the target side has would be lost. 4. Rewrites the mapped paths in the `.go` files the backport changed, only at the start of a string literal and on a path boundary. 5. Runs `gofmt` on files whose import blocks it resolved, then commits as diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index 4996900e..8376b134 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -32,12 +32,14 @@ from __future__ import annotations import base64 +import functools import os import re import shutil import subprocess import sys from pathlib import Path +from typing import Callable MARK_OURS = "<<<<<<< " MARK_BASE = "||||||| " @@ -53,7 +55,7 @@ # One import spec per line: optional name, quoted path, optional comment. IMPORT_SPEC = re.compile(r'^\s*(?:([A-Za-z_][A-Za-z0-9_]*|\.)\s+)?"([^"\s]+)"\s*(?://.*)?$') COMMENT = re.compile(r"^\s*//") -IDENT = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +PACKAGE_CLAUSE = re.compile(r"^package\s+([A-Za-z_][A-Za-z0-9_]*)", re.M) # Comments and string, raw string and rune literals: a name inside them is not a use. GO_NOISE = re.compile(r"""//[^\n]*|/\*.*?\*/|"(?:\\.|[^"\\\n])*"|`[^`]*`|'(?:\\.|[^'\\\n])*'""", re.S) @@ -126,22 +128,37 @@ def rewrite_paths(text: str, paths: list[tuple[str, str]]) -> str: return text -def import_name(line: str) -> str | None: - """Package name an import line binds, or None when it cannot be told - (blank and dot imports, paths whose name is not their last element).""" - m = IMPORT_SPEC.match(line) - if not m or m.group(1) in (".", "_"): - return None - if m.group(1): - return m.group(1) - last = m.group(2).split("/")[-1] - # A /vN element is either a module major (package named after its parent) - # or the package itself (k8s.io/api/core/v1 is package v1); the path - # cannot tell which. - if re.fullmatch(r"v\d+", last): - return None - last = re.sub(r"\.v\d+$", "", last) - return last if IDENT.fullmatch(last) else None +def package_clause(directory: Path) -> str | None: + """The package name the non-test .go files in a directory declare, or + None when there are none or they disagree.""" + names = set() + if directory.is_dir(): + for f in directory.glob("*.go"): + if not f.name.endswith("_test.go"): + m = PACKAGE_CLAUSE.search(GO_NOISE.sub(" ", f.read_text(encoding="utf-8", errors="replace"))) + if m: + names.add(m.group(1)) + return names.pop() if len(names) == 1 else None + + +def package_namer(owned: dict[str, tuple[str, str]], root: Path = Path(".")) -> Callable[[str], str | None]: + """Resolve an import path to the package name its source declares, since + the path does not say (github.com/json-iterator/go is package jsoniter). + The standard library goes by its last element, the repository's own + modules by their directory, everything else by vendor/. None when the + source is not in the checkout.""" + modules = sorted(owned.values(), key=lambda m: len(m[0]), reverse=True) + + @functools.cache + def name(path: str) -> str | None: + if "." not in path.split("/")[0]: + return [p for p in path.split("/") if not re.fullmatch(r"v\d+", p)][-1] + for mod_path, local in modules: + if path == mod_path or path.startswith(mod_path + "/"): + return package_clause(root / local / path[len(mod_path) + 1:]) + return package_clause(root / "vendor" / path) + + return name def is_import_only(lines: list[str]) -> bool: @@ -165,36 +182,49 @@ def block_end(lines: list[str], index: int) -> int: return len(lines) -def merge_imports(ours: list[str], theirs: list[str], body: str) -> list[str] | None: +def merge_imports(ours: list[str], theirs: list[str], body: str, + package_name: Callable[[str], str | None]) -> list[str] | None: """Theirs (the backported change) wins. An import only the target side - has stays while the code uses it. Returns None when that cannot be told - from the path, so the hunk is left for review: an unused import fails the - build as surely as a missing one.""" + has stays while the code uses it. Returns None whenever dropping or + keeping a target line cannot be proven safe, so the hunk is left for + review: an unused import fails the build as surely as a missing one.""" merged = list(theirs) - seen = {m.group(2): m.group(1) for m in map(IMPORT_SPEC.match, theirs) if m} + seen = {m.group(2): l.strip() for l in theirs if (m := IMPORT_SPEC.match(l))} + theirs_lines = {l.strip() for l in theirs} code = GO_NOISE.sub(" ", body) for line in ours: + if not line.strip(): + continue m = IMPORT_SPEC.match(line) if not m: + # A comment only the target side has would be lost. + if line.strip() not in theirs_lines: + return None continue - if m.group(2) in seen: - # Same path under another name (or blank/dot): the code outside - # the hunk may still use the target's name. - if seen[m.group(2)] != m.group(1): + alias, path = m.groups() + if path in seen: + # Same path under another name or comment (a //nolint, say): the + # code or a linter may rely on the target's line. + if seen[path] != line.strip(): return None continue - name = import_name(line) - if m.group(1) != "_" and name is None: + if alias == ".": return None - # Not after a dot: obj.config.Field is a field, not package config. - if m.group(1) == "_" or re.search(r"(? tuple[str, int]: +def resolve_import_hunks(text: str, paths: list[tuple[str, str]], + package_name: Callable[[str], str | None] = lambda _: None) -> tuple[str, int]: """Resolve conflict hunks made only of import lines inside an import block. Returns the new text and how many hunks were resolved.""" lines = text.split("\n") @@ -217,7 +247,7 @@ def resolve_import_hunks(text: str, paths: list[tuple[str, str]]) -> tuple[str, theirs = [rewrite_paths(l, paths) for l in lines[sep + 1:end]] merged = None if is_import_only(ours) and is_import_only(theirs) and in_import_block(lines, i): - merged = merge_imports(ours, theirs, "\n".join(lines[block_end(lines, end + 1) + 1:])) + merged = merge_imports(ours, theirs, "\n".join(lines[block_end(lines, end + 1) + 1:]), package_name) if merged is not None: out.extend(merged) resolved += 1 @@ -298,6 +328,7 @@ def run() -> dict[str, str]: git("checkout", "-q", "-B", branch, f"{remote}/{branch}") base = git("merge-base", f"{remote}/{target}", "HEAD").strip() changed, blocked = move_files(changed_files(base), dirs) + package_name = package_namer(owned_modules(gomods[1])) moved = git("diff", "--cached", "--name-only").strip() != "" rewritten, to_format, hunks = [], [], 0 @@ -305,7 +336,7 @@ def run() -> dict[str, str]: if path in blocked or not path.endswith(".go") or not Path(path).is_file(): continue before = Path(path).read_text(encoding="utf-8") - after, resolved = resolve_import_hunks(before, paths) + after, resolved = resolve_import_hunks(before, paths, package_name) after = rewrite_paths(after, paths) if after != before: Path(path).write_text(after, encoding="utf-8") diff --git a/.github/actions/backport-fix-imports/test/test_fix.py b/.github/actions/backport-fix-imports/test/test_fix.py index 0ba8e06c..d68d415c 100644 --- a/.github/actions/backport-fix-imports/test/test_fix.py +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -64,6 +64,7 @@ def repo(tmp_path, monkeypatch): subprocess.run(["git", "init", "-q", "-b", "release-4.12", str(work)], check=True) write(work, "go.mod", V4_GOMOD) write(work, f"{API_V4}/types/types.go", "package types\n") + write(work, "pkg/util/util.go", "package util\n\nvar X = 1\n") write(work, "pkg/peer/peer.go", 'package peer\n\nimport (\n\t"fmt"\n\n\t"example.com/acme/app/v4/pkg/util"\n)\n\nfunc A() { fmt.Println(util.X) }\n') commit_all(work, "release line") git(work, "checkout", "-q", "-b", "main") @@ -137,23 +138,52 @@ def test_rewrite_only_at_string_start_and_path_boundary(text, expected): assert fix.rewrite_paths(text, PATHS) == expected -@pytest.mark.parametrize("line, name", [ - ('\t"example.com/acme/app/v4/pkg/peer"', "peer"), - ('\tstoragev1 "example.com/acme/api/v4/pkg/apis/storage/v1"', "storagev1"), - ('\t"example.com/acme/api/v4"', None), - ('\t"k8s.io/api/core/v1"', None), - ('\t"gopkg.in/yaml.v3"', "yaml"), - ('\t"github.com/go-openapi/spec"', "spec"), - ('\t"github.com/x/go-openapi"', None), - ('\t_ "embed"', None), - ('\t. "example.com/dot"', None), +# -- package names --------------------------------------------------------- + +def go_file(root: Path, path: str, package: str) -> None: + (root / path).parent.mkdir(parents=True, exist_ok=True) + (root / path).write_text(f"// Package {package} does things.\npackage {package}\n") + + +@pytest.fixture +def namer(tmp_path): + go_file(tmp_path, "pkg/util/util.go", "util") + go_file(tmp_path, f"{API_V4}/pkg/x/x.go", "xapi") + go_file(tmp_path, "vendor/github.com/json-iterator/go/a.go", "jsoniter") + go_file(tmp_path, "vendor/k8s.io/api/core/v1/types.go", "v1") + go_file(tmp_path, "vendor/k8s.io/klog/v2/klog.go", "klog") + go_file(tmp_path, "vendor/example.com/mixed/a.go", "a") + go_file(tmp_path, "vendor/example.com/mixed/b.go", "b") + go_file(tmp_path, "vendor/example.com/tested/a_test.go", "tested_test") + go_file(tmp_path, "vendor/example.com/tested/a.go", "tested") + return fix.package_namer(fix.owned_modules(V4_GOMOD), tmp_path) + + +@pytest.mark.parametrize("path, name", [ + ("fmt", "fmt"), + ("net/http", "http"), + ("math/rand/v2", "rand"), + ("example.com/acme/app/v4/pkg/util", "util"), + ("example.com/acme/api/v4/pkg/x", "xapi"), + ("github.com/json-iterator/go", "jsoniter"), + ("k8s.io/api/core/v1", "v1"), + ("k8s.io/klog/v2", "klog"), + ("example.com/tested", "tested"), + # Not in the checkout, or files that disagree: no safe answer. + ("example.com/acme/app/v4/pkg/missing", None), + ("github.com/not/vendored", None), + ("example.com/mixed", None), ]) -def test_import_name(line, name): - assert fix.import_name(line) == name +def test_package_name_comes_from_the_source(namer, path, name): + assert namer(path) == name # -- resolving import hunks ------------------------------------------------ +def by_last_element(path: str) -> str: + return path.rsplit("/", 1)[-1] + + def conflicted(ours: list[str], theirs: list[str], body: str = "func F() {}\n") -> str: return "\n".join(["package p", "", "import (", '\t"fmt"', "", "<<<<<<< HEAD", *ours, "=======", *theirs, ">>>>>>> abc123 (feat: x (#1))", @@ -178,7 +208,7 @@ def test_target_only_import_kept_while_used_dropped_when_not(): ['\t"example.com/acme/app/v4/pkg/legacy"', '\t"example.com/acme/app/v4/pkg/gone"'], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { legacy.Run(); apply.X() }\n") - out, _ = fix.resolve_import_hunks(text, PATHS) + out, _ = fix.resolve_import_hunks(text, PATHS, by_last_element) assert '"example.com/acme/app/v4/pkg/legacy"' in out assert "pkg/gone" not in out @@ -186,7 +216,7 @@ def test_target_only_import_kept_while_used_dropped_when_not(): def test_field_access_is_not_a_package_use(): text = conflicted(['\t"example.com/acme/app/v4/pkg/config"'], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { x := obj.config.Field; apply.X(x) }\n") - out, _ = fix.resolve_import_hunks(text, PATHS) + out, _ = fix.resolve_import_hunks(text, PATHS, by_last_element) assert "pkg/config" not in out @@ -194,7 +224,7 @@ def test_field_access_is_not_a_package_use(): def test_package_use_forms_keep_the_import(use): text = conflicted(['\t"example.com/acme/app/v4/pkg/config"'], ['\t"example.com/acme/app/v5/pkg/apply"'], f"func F() {{ _ = {use}; apply.X() }}\n") - out, _ = fix.resolve_import_hunks(text, PATHS) + out, _ = fix.resolve_import_hunks(text, PATHS, by_last_element) assert '"example.com/acme/app/v4/pkg/config"' in out @@ -205,15 +235,13 @@ def test_blank_lines_between_import_groups_are_kept(): @pytest.mark.parametrize("line", [ - '\t"github.com/x/go-openapi"', - # k8s.io/api/core/v1 is package v1, k8s.io/klog/v2 is package klog: the - # path alone cannot tell, so neither keeping nor dropping is safe. + '\t"github.com/json-iterator/go"', '\t"k8s.io/api/core/v1"', - '\t"k8s.io/klog/v2"', '\t. "example.com/dot"', ]) def test_target_only_import_with_unknown_name_leaves_the_hunk(line): - text = conflicted([line], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { _ = v1.Pod{}; apply.X() }\n") + # No source to read the package name from: neither keeping nor dropping is safe. + text = conflicted([line], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { jsoniter.X(); apply.X() }\n") out, resolved = fix.resolve_import_hunks(text, PATHS) assert resolved == 0 assert out == text @@ -240,6 +268,42 @@ def test_same_path_same_name_is_deduplicated(): assert out.count('cfg "example.com/acme/app/v4/pkg/config"') == 1 +def test_package_name_from_source_decides_the_import(namer): + # github.com/json-iterator/go is package jsoniter, not go. + text = conflicted(['\t"github.com/json-iterator/go"', '\t"k8s.io/klog/v2"'], + ['\t"example.com/acme/app/v5/pkg/util"'], "func F() { jsoniter.X(); util.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS, namer) + assert resolved == 1 + assert '"github.com/json-iterator/go"' in out + assert "klog" not in out + + +@pytest.mark.parametrize("ours, theirs", [ + ('\t"example.com/acme/app/v4/legacy" //nolint:staticcheck', '\t"example.com/acme/app/v5/legacy"'), + ('\t"example.com/acme/app/v4/legacy"', '\t"example.com/acme/app/v5/legacy" // new note'), +]) +def test_same_path_with_another_comment_leaves_the_hunk(ours, theirs): + text = conflicted([ours], [theirs], "func F() { legacy.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS, by_last_element) + assert resolved == 0 + assert out == text + + +def test_comment_line_only_the_target_has_leaves_the_hunk(): + text = conflicted(["\t// keep first: registers the scheme", '\t"example.com/acme/app/v4/pkg/a"'], + ['\t"example.com/acme/app/v5/pkg/a"'], "func F() { a.X() }\n") + _, resolved = fix.resolve_import_hunks(text, PATHS, by_last_element) + assert resolved == 0 + + +def test_comment_line_both_sides_have_is_fine(): + text = conflicted(["\t// shared note", '\t"example.com/acme/app/v4/pkg/a"'], + ["\t// shared note", '\t"example.com/acme/app/v5/pkg/a"'], "func F() { a.X() }\n") + out, resolved = fix.resolve_import_hunks(text, PATHS, by_last_element) + assert resolved == 1 + assert out.count("// shared note") == 1 + + def test_target_only_blank_import_is_kept(): text = conflicted(['\t_ "embed"'], ['\t"example.com/acme/app/v5/pkg/apply"'], "func F() { apply.X() }\n") out, resolved = fix.resolve_import_hunks(text, PATHS) @@ -255,7 +319,7 @@ def test_target_only_blank_import_is_kept(): ]) def test_use_in_comment_or_string_does_not_keep_the_import(body): text = conflicted(['\t"example.com/acme/app/v4/pkg/config"'], ['\t"example.com/acme/app/v5/pkg/apply"'], body) - out, resolved = fix.resolve_import_hunks(text, PATHS) + out, resolved = fix.resolve_import_hunks(text, PATHS, by_last_element) assert resolved == 1 assert "pkg/config" not in out From 57bc1a7c96d34824637472e0f7a53b6f918480b0 Mon Sep 17 00:00:00 2001 From: Caue Santos Date: Thu, 22 Oct 2026 13:57:21 -0600 Subject: [PATCH 7/7] refactor(backport-fix-imports): keep only conclusion and reason outputs --- .../actions/backport-fix-imports/README.md | 23 ++++++++----------- .../actions/backport-fix-imports/action.yml | 12 ---------- .../actions/backport-fix-imports/src/fix.py | 17 +++++--------- .../backport-fix-imports/test/test_fix.py | 11 ++++----- 4 files changed, 20 insertions(+), 43 deletions(-) diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md index 3dca4977..506d3c8f 100644 --- a/.github/actions/backport-fix-imports/README.md +++ b/.github/actions/backport-fix-imports/README.md @@ -37,7 +37,8 @@ already exists on the remote: `fix(backport): use module paths`. Conflict hunks in real code, undecidable import hunks and blocked moves are -left as they are and counted in `conflicts-remaining`, with a warning per file. A second run finds nothing to +left as they are, with a warning per file, and the `reason` says how many files +are left for review. A second run finds nothing to rewrite and reports `unchanged`, so re-runs are safe. The action never fails the job: every failure path ends with `conclusion=failed`, @@ -58,10 +59,10 @@ a one-line `reason` and exit 0, and nothing is pushed. backport-branch: backport/release-4.12/pr-8606 github-token: ${{ secrets.GH_ACCESS_TOKEN }} -- if: >- - (steps.fix.outputs.conclusion == 'fixed' || steps.fix.outputs.conclusion == 'unchanged') && - steps.fix.outputs.conflicts-remaining != '0' - run: echo "conflicts left for review" +- env: + CONCLUSION: ${{ steps.fix.outputs.conclusion }} + REASON: ${{ steps.fix.outputs.reason }} + run: echo "$CONCLUSION: $REASON" ``` The runner needs `python3` and `git`; `gofmt` is optional and only formats the @@ -83,14 +84,10 @@ the backport PR's CI. -| OUTPUT | TYPE | DESCRIPTION | -|---------------------|--------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| commit | string | SHA of the fix commit; empty
when none was made. | -| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | -| conflicts-remaining | string | Number of changed files left for
a person: conflict markers remain, or
a new file could not be
moved because its target path exists.
0 on skipped and failed, where
nothing was checked, so gate on
conclusion too. | -| files-changed | string | Number of .go files rewritten. | -| hunks-resolved | string | Number of import-only conflict hunks resolved. | -| reason | string | One-line explanation of the conclusion. | +| OUTPUT | TYPE | DESCRIPTION | +|------------|--------|-------------------------------------------------------------------------------------------------------------------------------------| +| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (same module paths, no go.mod, or missing input)
or failed. | +| reason | string | One-line explanation of the conclusion. | diff --git a/.github/actions/backport-fix-imports/action.yml b/.github/actions/backport-fix-imports/action.yml index 6f51920b..d3c00c86 100644 --- a/.github/actions/backport-fix-imports/action.yml +++ b/.github/actions/backport-fix-imports/action.yml @@ -33,18 +33,6 @@ outputs: reason: description: 'One-line explanation of the conclusion.' value: ${{ steps.fix.outputs.reason || 'the fix step did not run; see its log' }} - files-changed: - description: 'Number of .go files rewritten.' - value: ${{ steps.fix.outputs.files-changed || '0' }} - hunks-resolved: - description: 'Number of import-only conflict hunks resolved.' - value: ${{ steps.fix.outputs.hunks-resolved || '0' }} - conflicts-remaining: - description: 'Number of changed files left for a person: conflict markers remain, or a new file could not be moved because its target path exists. 0 on skipped and failed, where nothing was checked, so gate on conclusion too.' - value: ${{ steps.fix.outputs.conflicts-remaining || '0' }} - commit: - description: 'SHA of the fix commit; empty when none was made.' - value: ${{ steps.fix.outputs.commit }} runs: using: composite diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index 8376b134..9ebc6262 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -349,24 +349,21 @@ def run() -> dict[str, str]: marked = [p for p in changed if Path(p).is_file() and has_markers(Path(p).read_text(encoding="utf-8", errors="replace"))] for p in marked: print(f"::warning::{p}: conflict markers remain; resolve them by hand") - remaining = sorted(set(marked) | set(blocked)) - result = {"files-changed": str(len(rewritten)), "hunks-resolved": str(hunks), - "conflicts-remaining": str(len(remaining)), "commit": ""} + left = len(set(marked) | set(blocked)) + left_note = f"; {left} file(s) left for review" if left else "" if not rewritten and not moved: - return {**result, "conclusion": "unchanged", "reason": "no module paths to rewrite"} + return {"conclusion": "unchanged", "reason": f"no module paths to rewrite{left_note}"} if rewritten: git("add", "--", *rewritten) mapping = ", ".join(f"{s} -> {t}" for s, t in paths) git("-c", f"user.name={BOT_NAME}", "-c", f"user.email={BOT_EMAIL}", "commit", "-q", "-m", f"fix(backport): use {target} module paths", "-m", f"Rewrote {mapping}.") - sha = git("rev-parse", "HEAD").strip() - result["commit"] = sha if push: git("push", "-q", remote, f"HEAD:refs/heads/{branch}") - reason = f"rewrote {len(rewritten)} file(s), resolved {hunks} import conflict(s)" - return {**result, "conclusion": "fixed", "reason": reason} + reason = f"rewrote {len(rewritten)} file(s), resolved {hunks} import conflict(s){left_note}" + return {"conclusion": "fixed", "reason": reason} def main() -> int: @@ -378,9 +375,7 @@ def main() -> int: except Exception as e: # noqa: BLE001 -- advisory: never fail the backport result = {"conclusion": "failed", "reason": str(e).splitlines()[0] if str(e) else type(e).__name__} print(f"::warning::backport-fix-imports failed: {result['reason']}; the backport PR is unchanged, fix it by hand") - # Every output on every path: an empty count would read as "not 0". - defaults = {"files-changed": "0", "hunks-resolved": "0", "conflicts-remaining": "0", "commit": ""} - for key, value in {**defaults, **result}.items(): + for key, value in result.items(): emit(key, value) print(f"conclusion={result['conclusion']} reason={result['reason']}") return 0 diff --git a/.github/actions/backport-fix-imports/test/test_fix.py b/.github/actions/backport-fix-imports/test/test_fix.py index d68d415c..f4283626 100644 --- a/.github/actions/backport-fix-imports/test/test_fix.py +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -360,7 +360,6 @@ def test_clean_pick_with_source_imports_is_rewritten_and_pushed(repo): assert fix.main() == 0 out = outputs(repo) assert out["conclusion"] == "fixed" - assert out["files-changed"] == "1" content = remote_file(repo, "pkg/peer/peer_test.go") assert '"example.com/acme/api/v4/pkg/apis/storage/v1"' in content assert '"example.com/acme/app/v4/pkg/peer/networkpeer"' in content @@ -398,8 +397,8 @@ def test_move_blocked_by_existing_target_is_left_for_review(repo): push_backport(repo) fix.main() out = outputs(repo) - assert out["conflicts-remaining"] == "1" assert out["conclusion"] == "unchanged" + assert "1 file(s) left for review" in out["reason"] assert '"example.com/acme/api/v5/pkg/x"' in remote_file(repo, f"{API_V5}/types/types.go") @@ -424,8 +423,7 @@ def test_import_conflict_resolved_code_conflict_counted(repo): fix.main() out = outputs(repo) assert out["conclusion"] == "fixed" - assert out["hunks-resolved"] == "1" - assert out["conflicts-remaining"] == "1" + assert "resolved 1 import conflict(s); 1 file(s) left for review" in out["reason"] assert not fix.has_markers(remote_file(repo, "pkg/peer/peer.go")) assert fix.has_markers(remote_file(repo, "pkg/peer/code.go")) @@ -470,7 +468,8 @@ def test_push_false_leaves_the_remote_alone(repo, monkeypatch): before = git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") fix.main() out = outputs(repo) - assert out["conclusion"] == "fixed" and out["commit"] != before + assert out["conclusion"] == "fixed" + assert git(repo, "rev-parse", "HEAD") != before git(repo, "fetch", "-q", "origin") assert git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") == before @@ -481,7 +480,6 @@ def test_missing_branch_fails_soft(repo, monkeypatch): out = outputs(repo) assert out["conclusion"] == "failed" assert "git fetch failed" in out["reason"] - assert out["conflicts-remaining"] == "0" and out["commit"] == "" def test_missing_inputs_skip(repo, monkeypatch): @@ -489,4 +487,3 @@ def test_missing_inputs_skip(repo, monkeypatch): assert fix.main() == 0 out = outputs(repo) assert out["conclusion"] == "skipped" - assert out["files-changed"] == out["hunks-resolved"] == out["conflicts-remaining"] == "0"