diff --git a/.github/actions/backport-fix-imports/README.md b/.github/actions/backport-fix-imports/README.md index 506d3c8f..8fa742e0 100644 --- a/.github/actions/backport-fix-imports/README.md +++ b/.github/actions/backport-fix-imports/README.md @@ -44,6 +44,19 @@ 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. +## PR comment + +When there is something to tell the assignee, the action upserts one sticky +comment on the backport PR: the fix commit, and each file left for them with +the reason. No comment when nothing was changed and nothing is left. + +## In the backport workflow + +[`backport.yaml`](../../../docs/workflows/backport.md) runs this action on +every backport branch after the cherry-picks when the caller sets +`fix-imports: true`. Off by default. A label with no backport branch, such as +a legacy line, is skipped. + ## Usage ```yaml @@ -84,10 +97,10 @@ the backport PR's CI. -| 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. | +| OUTPUT | TYPE | DESCRIPTION | +|------------|--------|---------------------------------------------------------------------------------------------------------------------------------------------------------| +| conclusion | string | fixed (a commit was made), unchanged (nothing to rewrite), skipped (no backport branch, 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 d3c00c86..bd10a788 100644 --- a/.github/actions/backport-fix-imports/action.yml +++ b/.github/actions/backport-fix-imports/action.yml @@ -28,7 +28,7 @@ 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.' + description: 'fixed (a commit was made), unchanged (nothing to rewrite), skipped (no backport branch, same module paths, no go.mod, or missing input) or failed.' value: ${{ steps.fix.outputs.conclusion || 'failed' }} reason: description: 'One-line explanation of the conclusion.' @@ -52,3 +52,26 @@ runs: PUSH: ${{ inputs.push }} GH_TOKEN: ${{ inputs.github-token }} run: python3 "${{ github.action_path }}/src/fix.py" + + - name: Write the backport PR comment + id: report + # A dry run pushes nothing, so it must not post a comment naming its commit. + if: (steps.fix.outputs.conclusion == 'fixed' || steps.fix.outputs.conclusion == 'unchanged') && inputs.push != 'false' + continue-on-error: true + shell: bash + env: + BACKPORT_BRANCH: ${{ inputs.backport-branch }} + GH_TOKEN: ${{ inputs.github-token }} + run: python3 "${{ github.action_path }}/src/report.py" + + - name: Comment on the backport PR + if: steps.report.outputs.body != '' + continue-on-error: true + # Pinned by SHA like every internal composite (docs/CONVENTIONS.md). + uses: loft-sh/github-actions/.github/actions/sticky-pr-comment@d462e847d4f8f4a139890264a35b68cc30472547 # sticky-pr-comment/v1 + with: + marker: '' + body: ${{ steps.report.outputs.body }} + pr-number: ${{ steps.report.outputs.pr-number }} + github-token: ${{ inputs.github-token }} + expected-author: ${{ steps.report.outputs.author }} diff --git a/.github/actions/backport-fix-imports/src/fix.py b/.github/actions/backport-fix-imports/src/fix.py index 9ebc6262..0a4ab5f1 100644 --- a/.github/actions/backport-fix-imports/src/fix.py +++ b/.github/actions/backport-fix-imports/src/fix.py @@ -33,11 +33,14 @@ import base64 import functools +import json import os import re import shutil import subprocess import sys +import urllib.parse +import urllib.request from pathlib import Path from typing import Callable @@ -59,6 +62,7 @@ # 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) +NOTES_FILE = "backport-fix-imports-notes.json" BOT_NAME = "github-actions[bot]" BOT_EMAIL = "41898282+github-actions[bot]@users.noreply.github.com" @@ -87,6 +91,38 @@ def git(*args: str, check: bool = True) -> str: return res.stdout +def tool_env() -> dict[str, str]: + """The job environment without the push token: gofmt never needs it.""" + return {k: v for k, v in os.environ.items() if k not in ("GH_TOKEN", "GITHUB_TOKEN")} + + +def note(section: str, data: dict) -> None: + """Record what a pass did, for the PR comment. A no-op outside Actions.""" + if not os.environ.get("RUNNER_TEMP"): + return + path = Path(os.environ["RUNNER_TEMP"]) / NOTES_FILE + notes = json.loads(path.read_text()) if path.is_file() else {} + notes[section] = data + path.write_text(json.dumps(notes)) + + +def github(method: str, path: str, body: dict | None = None): + req = urllib.request.Request( + f"https://api.github.com{path}", method=method, + data=json.dumps(body).encode() if body is not None else None, + headers={"Authorization": f"Bearer {os.environ['GH_TOKEN']}", + "Accept": "application/vnd.github+json"}) + with urllib.request.urlopen(req, timeout=30) as res: + return json.load(res) + + +def backport_pr(branch: str) -> int | None: + repo = os.environ["GITHUB_REPOSITORY"] + head = urllib.parse.quote(f"{repo.split('/')[0]}:{branch}") + prs = github("GET", f"/repos/{repo}/pulls?state=open&head={head}") + return prs[0]["number"] if prs else None + + def strip_major(path: str) -> str: return MAJOR_SUFFIX.sub("", path) @@ -299,7 +335,7 @@ def gofmt(files: list[str]) -> None: 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) + res = subprocess.run(["gofmt", "-w", *files], capture_output=True, text=True, env=tool_env()) if res.returncode != 0: raise RuntimeError(f"gofmt rejected the result, nothing committed: {res.stderr.strip()}") @@ -313,6 +349,9 @@ def run() -> dict[str, str]: if not target or not branch: raise Skip("target-branch and backport-branch are required; pass both from the backport job") + # A label with no cherry-pick branch (a legacy line, a failed pick) is not ours to fix. + if not git("ls-remote", "--heads", remote, f"refs/heads/{branch}").strip(): + raise Skip(f"no {branch} branch on {remote}; nothing to fix") git("fetch", "-q", "--no-tags", remote, *(f"+refs/heads/{b}:refs/remotes/{remote}/{b}" for b in (target, source, branch))) gomods = [] @@ -349,10 +388,12 @@ 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") - left = len(set(marked) | set(blocked)) - left_note = f"; {left} file(s) left for review" if left else "" + left = {p: "conflict markers remain" for p in marked} + left.update({p: "added under the source path, but that path exists on the target" for p in blocked}) + left_note = f"; {len(left)} file(s) left for review" if left else "" if not rewritten and not moved: + note("script", {"rewritten": 0, "hunks": 0, "left": left, "commit": ""}) return {"conclusion": "unchanged", "reason": f"no module paths to rewrite{left_note}"} if rewritten: @@ -362,11 +403,15 @@ def run() -> dict[str, str]: "-m", f"fix(backport): use {target} module paths", "-m", f"Rewrote {mapping}.") if push: git("push", "-q", remote, f"HEAD:refs/heads/{branch}") + note("script", {"rewritten": len(rewritten), "hunks": hunks, "left": left, + "commit": git("rev-parse", "HEAD").strip()}) reason = f"rewrote {len(rewritten)} file(s), resolved {hunks} import conflict(s){left_note}" return {"conclusion": "fixed", "reason": reason} def main() -> int: + if os.environ.get("RUNNER_TEMP"): + (Path(os.environ["RUNNER_TEMP"]) / NOTES_FILE).unlink(missing_ok=True) try: result = run() except Skip as e: diff --git a/.github/actions/backport-fix-imports/src/report.py b/.github/actions/backport-fix-imports/src/report.py new file mode 100644 index 00000000..f04511b5 --- /dev/null +++ b/.github/actions/backport-fix-imports/src/report.py @@ -0,0 +1,65 @@ +#!/usr/bin/env python3 +"""backport-fix-imports: turn what the fix did into the backport PR comment. + +Reads the notes fix.py left in RUNNER_TEMP and writes body, pr-number and +author to GITHUB_OUTPUT for sticky-pr-comment. Writes nothing when there is +nothing to tell the assignee. Never fails the job. + +Env: + BACKPORT_BRANCH, GH_TOKEN as for fix.py + RUNNER_TEMP where the notes are +""" +from __future__ import annotations + +import json +import os +import secrets +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import fix # noqa: E402 + + +def count(n: int, noun: str) -> str: + return f"{n} {noun}" if n == 1 else f"{n} {noun}s" + + +def body(notes: dict) -> str: + script = notes.get("script", {}) + lines = [] + if script.get("commit"): + lines.append(f"- Module paths: fixed in {script['commit'][:10]} ({count(script.get('rewritten', 0), 'file')} " + f"rewritten, {count(script.get('hunks', 0), 'import conflict')} resolved).") + left = script.get("left", {}) + if left: + lines.append("- Left for you:") + lines.extend(f" - `{p}`: {r}" for p, r in sorted(left.items())) + return "\n".join(["### Import fixes for this backport", *lines]) if lines else "" + + +def main() -> int: + try: + path = Path(os.environ.get("RUNNER_TEMP", ".")) / fix.NOTES_FILE + text = body(json.loads(path.read_text())) if path.is_file() else "" + if not text: + return 0 + pr = fix.backport_pr(os.environ["BACKPORT_BRANCH"]) + if pr is None: + print("::warning::no open backport PR to comment on") + return 0 + # sticky-pr-comment only updates comments by this author. Read it + # first, so a failed lookup writes no outputs and no comment step runs. + author = fix.github("GET", "/user")["login"] + delim = f"EOF_{secrets.token_hex(8)}" + with open(os.environ.get("GITHUB_OUTPUT", os.devnull), "a", encoding="utf-8") as f: + f.write(f"body<<{delim}\n{text}\n{delim}\n") + fix.emit("pr-number", str(pr)) + fix.emit("author", author) + except Exception as e: # noqa: BLE001 -- advisory: never fail the backport + print(f"::warning::backport PR comment skipped: {str(e).splitlines()[0] if str(e) else type(e).__name__}") + 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 index f4283626..b0d644e7 100644 --- a/.github/actions/backport-fix-imports/test/test_fix.py +++ b/.github/actions/backport-fix-imports/test/test_fix.py @@ -3,6 +3,7 @@ from __future__ import annotations import importlib.util +import json import shutil import subprocess from pathlib import Path @@ -23,7 +24,7 @@ def load(): V4_GOMOD = """module example.com/acme/app/v4 -go 1.26 +go 1.22 require example.com/acme/api/v4 v4.0.0 @@ -366,6 +367,17 @@ def test_clean_pick_with_source_imports_is_rewritten_and_pushed(repo): assert git(repo, "log", "-1", "--format=%s", "origin/backport/release-4.12/pr-1") == "fix(backport): use release-4.12 module paths" +def test_script_notes_what_it_fixed_and_left(repo, monkeypatch): + monkeypatch.setenv("RUNNER_TEMP", str(repo.parent)) + write(repo, "pkg/peer/new.go", 'package peer\n\nimport "example.com/acme/app/v5/pkg/util"\n\nvar _ = util.X\n') + 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() + notes = json.loads((repo.parent / fix.NOTES_FILE).read_text())["script"] + assert notes["rewritten"] == 1 and notes["commit"] + assert notes["left"] == {"pkg/peer/code.go": "conflict markers remain"} + + 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') @@ -392,10 +404,13 @@ def test_existing_file_under_source_dir_is_not_moved(repo): assert f"{API_V4}/types/kept.go" not in tree -def test_move_blocked_by_existing_target_is_left_for_review(repo): +def test_move_blocked_by_existing_target_is_left_for_review(repo, monkeypatch): + monkeypatch.setenv("RUNNER_TEMP", str(repo.parent)) 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() + left = json.loads((repo.parent / fix.NOTES_FILE).read_text())["script"]["left"] + assert left == {f"{API_V5}/types/types.go": "added under the source path, but that path exists on the target"} out = outputs(repo) assert out["conclusion"] == "unchanged" assert "1 file(s) left for review" in out["reason"] @@ -474,9 +489,17 @@ def test_push_false_leaves_the_remote_alone(repo, monkeypatch): assert git(repo, "rev-parse", "origin/backport/release-4.12/pr-1") == before -def test_missing_branch_fails_soft(repo, monkeypatch): +def test_missing_backport_branch_skips(repo, monkeypatch): monkeypatch.setenv("BACKPORT_BRANCH", "backport/release-4.12/pr-404") assert fix.main() == 0 + assert outputs(repo)["conclusion"] == "skipped" + + +def test_missing_target_branch_fails_soft(repo, monkeypatch): + write(repo, "pkg/peer/new.go", "package peer\n") + push_backport(repo) + monkeypatch.setenv("TARGET_BRANCH", "release-9.9") + assert fix.main() == 0 out = outputs(repo) assert out["conclusion"] == "failed" assert "git fetch failed" in out["reason"] diff --git a/.github/actions/backport-fix-imports/test/test_report.py b/.github/actions/backport-fix-imports/test/test_report.py new file mode 100644 index 00000000..b4d43974 --- /dev/null +++ b/.github/actions/backport-fix-imports/test/test_report.py @@ -0,0 +1,95 @@ +"""report.py: the comment body from the fix's notes, and the outputs it hands +sticky-pr-comment. GitHub is faked.""" +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path + +import pytest + +SRC = Path(__file__).resolve().parent.parent / "src" +sys.path.insert(0, str(SRC)) +spec = importlib.util.spec_from_file_location("backport_report", SRC / "report.py") +report = importlib.util.module_from_spec(spec) +spec.loader.exec_module(report) + +SCRIPT = {"rewritten": 22, "hunks": 18, "commit": "b2c250ffb0050cae", "left": { + "pkg/a/a_test.go": "conflict markers remain", "pkg/b/b.go": "conflict markers remain"}} + + +def test_nothing_to_tell_means_no_comment(): + assert report.body({}) == "" + assert report.body({"script": {"rewritten": 0, "hunks": 0, "commit": "", "left": {}}}) == "" + + +def test_fix_and_leftovers(): + text = report.body({"script": SCRIPT}) + assert "fixed in b2c250ffb0 (22 files rewritten, 18 import conflicts resolved)" in text + assert " - `pkg/a/a_test.go`: conflict markers remain" in text + assert " - `pkg/b/b.go`: conflict markers remain" in text + + +def test_counts_are_singular_for_one(): + text = report.body({"script": {"rewritten": 1, "hunks": 1, "commit": "22cb3caca00f", "left": {}}}) + assert "(1 file rewritten, 1 import conflict resolved)" in text + zero = report.body({"script": {"rewritten": 0, "hunks": 0, "commit": "22cb3caca00f", "left": {}}}) + assert "(0 files rewritten, 0 import conflicts resolved)" in zero + + +def test_only_leftovers(): + text = report.body({"script": {"rewritten": 0, "hunks": 0, "commit": "", + "left": {"pkg/b/b.go": "conflict markers remain"}}}) + assert "Module paths" not in text and "`pkg/b/b.go`" in text + + +@pytest.fixture +def env(tmp_path, monkeypatch): + monkeypatch.setenv("RUNNER_TEMP", str(tmp_path)) + monkeypatch.setenv("GITHUB_OUTPUT", str(tmp_path / "output")) + monkeypatch.setenv("BACKPORT_BRANCH", "backport/release-4.12/pr-1") + monkeypatch.setattr(report.fix, "backport_pr", lambda branch: 42) + monkeypatch.setattr(report.fix, "github", lambda method, path, body=None: {"login": "acme-bot"}) + return tmp_path + + +def test_main_hands_sticky_pr_comment_its_inputs(env): + (env / report.fix.NOTES_FILE).write_text(json.dumps({"script": SCRIPT})) + assert report.main() == 0 + out = (env / "output").read_text() + assert out.startswith("body<- + After the cherry-picks, run backport-fix-imports on each backport + branch: rewrite the repository's own Go module paths to the target + line's major, resolve import-only conflicts, and push a fix commit. + For repositories whose release lines sit on an older module major. + type: boolean + required: false + default: false secrets: gh-access-token: description: 'GitHub PAT for creating backport PRs' @@ -70,7 +79,16 @@ jobs: timeout-minutes: 20 permissions: contents: read + outputs: + targets: ${{ steps.targets.outputs.targets }} steps: + # First, so fix-imports gets its targets even when a cherry-pick fails. + - name: List backport targets + id: targets + env: + LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} + run: echo "targets=$(jq -c '[.[] | select(startswith("backport-to-")) | ltrimstr("backport-to-")]' <<< "$LABELS")" >> "$GITHUB_OUTPUT" + - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -102,6 +120,36 @@ jobs: if: ${{ failure() }} run: cat ~/.backport/backport.debug.log + # One job per backport-to-* label. A label with no cherry-pick branch (a + # legacy line, a failed pick) makes the action skip, so the matrix needs no + # filtering here. + fix-imports: + name: Fix imports for ${{ matrix.target }} + needs: backport + if: inputs.fix-imports && !cancelled() && needs.backport.outputs.targets != '' && needs.backport.outputs.targets != '[]' + runs-on: ubuntu-latest + timeout-minutes: 30 + permissions: + contents: read + strategy: + fail-fast: false + matrix: + target: ${{ fromJSON(needs.backport.outputs.targets) }} + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Fix imports on backport/${{ matrix.target }}/pr-${{ github.event.pull_request.number }} + uses: loft-sh/github-actions/.github/actions/backport-fix-imports@backport-fix-imports/v1 # zizmor: ignore[unpinned-uses] -- internal loft-sh/github-actions ref; reusable workflows check out the CALLER repo, so a local ./.github/actions path is not present here. @backport-fix-imports/v1 is the floating coordination tag kept aligned with backport/v1 (see DEVOPS-923 pin-drift class). + with: + target-branch: ${{ matrix.target }} + backport-branch: backport/${{ matrix.target }}/pr-${{ github.event.pull_request.number }} + source-branch: ${{ github.event.pull_request.base.ref }} + github-token: ${{ secrets.gh-access-token }} # zizmor: ignore[secrets-outside-env] -- PAT passed through workflow_call; pushes the fix commit so the backport PR's CI runs on it + # --- pre-monorepo (<= v0.36) split/re-root flow --------------------------- # sorenlouv (above) only cherry-picks monorepo-era (>= v0.37) targets. Legacy # lines predate the monorepo layout, so a cherry-pick there would recreate the diff --git a/README.md b/README.md index 693e2aab..e1cfe25d 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,7 @@ See [select-backport-source-prs README](./.github/actions/select-backport-source ### 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. +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 for the assignee, listed in a sticky PR comment. Wired into [`backport.yaml`](./docs/workflows/backport.md) behind `fix-imports`; advisory, never fails the job. **Location:** `.github/actions/backport-fix-imports` diff --git a/docs/workflows/backport.md b/docs/workflows/backport.md index cbee38ae..5ec94462 100644 --- a/docs/workflows/backport.md +++ b/docs/workflows/backport.md @@ -9,6 +9,7 @@ Creates backport PRs when a merged PR is labeled with `backport/`. | INPUT | TYPE | REQUIRED | DEFAULT | DESCRIPTION | |----------------|---------|----------|---------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | docs-url | string | false | | Optional link to this repo's backport
documentation. Added to the failure comment
so whoever hits it knows where
the process is written down. Omitted
when empty. An input rather than
a constant because this workflow is
shared with public repositories. | +| fix-imports | boolean | false | `false` | After the cherry-picks, run backport-fix-imports on
each backport branch: rewrite the repository's
own Go module paths to the
target line's major, resolve import-only conflicts,
and push a fix commit. For
repositories whose release lines sit on
an older module major. | | legacy-split | boolean | false | `false` | Enable the pre-monorepo (<= v0.36) split/re-root backport
flow. When true this workflow empties
auto_backport_label_prefix so sorenlouv honors the caller's
.backportrc.json branchLabelMapping regex (which MUST be scoped to the monorepo era, >= v0.37), and a
companion job routes <= v0.36 targets
through backport-legacy-split. Requires subtree-prefix, oss-repo and
pro-repo. | | oss-repo | string | false | | OSS repo as owner/repo for legacy
OSS-side backport PRs. Required when legacy-split
is true. | | pro-repo | string | false | | Pro repo as owner/repo for the
pro half of mixed legacy backports.
Required when legacy-split is true. |