diff --git a/README.md b/README.md index 557d801..35f95e5 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ fetch it — which was measured to be the difference between one step and three. | file | what it does | |---|---| | `with-nixpkgs.sh` | Runs a command with one nixpkgs package on PATH, pinned to the *consuming* repo's `flake.lock`. Avoids `nix shell nixpkgs#x`, which re-resolves the registry and refetches a channel tarball whenever the branch moves. | -| `report_job_log.py` | Posts the tail of a build log as a PR comment. Exists because `actions/jobs/{id}/logs` returns 500 for every id on Gitea 1.25.2, so a red job otherwise says only that it failed. Takes `--status`; see below. | +| `report_job_log.py` | Posts the tail of a build log as a PR comment, falling back to a commit status when a `push`-triggered job has no PR to comment on. Exists because `actions/jobs/{id}/logs` returns 500 for every id on Gitea 1.25.2, so a red job otherwise says only that it failed. Takes `--status`; see below. | | `sync_blocked_label.py` | Keeps `Status/Blocked` in step with Gitea's dependency graph. Resolves the label from the repo *or the organisation*, never touches an issue marked `Status/On Hold` or `Status/Abandoned`, and leaves the label alone (rather than guessing) on a blocker whose state a repo-scoped token cannot resolve — see weblib-ci#11. | All three are standard library / plain bash only. They are *run*, not built, so @@ -27,7 +27,7 @@ test reads state back rather than trusting an exit status of 0 (which the reporter in particular returns even when its POST failed). ```bash -python3 test_report_job_log.py # 22 tests, ~1.2s, no network +python3 test_report_job_log.py # 28 tests, ~3s, no network python3 test_sync_blocked_label.py # 12 tests, no network ``` @@ -100,6 +100,50 @@ status is put in the header verbatim rather than rejected: `argparse`'s `choices=` would exit 2 on a value this list has not heard of, and the log — the entire reason the script exists — would never be posted. +### `push`-triggered jobs: no open PR does not mean no channel + +Found the hard way on weblib-ci#14: a `push` to `development`/`main` — the +common shape of a workflow like a publish pipeline, which has no +`pull_request` trigger at all — has no *open* PR for `report_job_log.py` to +comment on. The branch that was pushed to is the merge's **target**, not the +PR that produced it, and that PR is already closed. The old behaviour was to +print "no pull request for this run" and exit 0, so a failure there was +completely invisible: not the log (500 on this Gitea), not a comment +(nowhere to put it), nothing. That is how weblib-viewer run 1769's failure +(`publish.sh` refusing to overwrite an existing APK) went unnoticed until it +was reproduced by hand. + +Two things changed, tried in order, neither needing a new flag: + +1. **Ask Gitea which PR produced this exact commit.** + `GET repos/{repo}/commits/{sha}/pull` resolves the PR a commit came from — + open or closed — keyed on the commit itself rather than a branch name. + Verified against both merge styles used in this org: an explicit merge + commit, and a fast-forward promotion where the "merge commit" is just the + PR branch's own tip. This needs the **full** 40-character `GITHUB_SHA`; a + short sha gets the same 404 shape as a commit with no PR at all, so it + would silently look like "not found". This covers the ordinary case — + almost every push to a protected branch here is a merge landing. +2. **If that also finds nothing** — a commit that was never part of any + PR — post a **commit status** instead + (`POST repos/{repo}/statuses/{sha}`, context `report_job_log`). It is not + a substitute for the full log: this Gitea did not truncate a 3000-character + description in testing, but a status is UI chrome, not a page, so what + goes there is deliberately a short snippet (`STATUS_TAIL`, 500 characters), + not the whole tail. `state` follows the same outcome as the header phrase + (`failed → failure`, `passed → success`, `cancelled → warning`, + `skipped → skipped`; an unrecognised status maps to `error` rather than + guessing pass or fail), and `target_url` points at the commit. + +**A commit *comment* API (GitHub-style, `commits/{sha}/comments`) does not +exist on this Gitea** — 404, and absent from `/swagger.v1.json` entirely, not +merely unauthorised. That ruled out the most direct-looking option; the +commit-status endpoint is the one that is actually there. + +If a PR *is* found (by either the original lookups or the new sha lookup), +behaviour is identical to before: a comment, same format. The status path is +only reached when no PR exists anywhere for the commit. + ### Why not a flake input These are scripts a workflow runs, not derivations. A flake input would cost a diff --git a/report_job_log.py b/report_job_log.py index d589dab..67e97af 100755 --- a/report_job_log.py +++ b/report_job_log.py @@ -28,6 +28,16 @@ Everything else comes from the Actions environment: `GITHUB_REPOSITORY`, `GITHUB_EVENT_PATH` (for the PR number), `GITEA_TOKEN`/`GITHUB_TOKEN`, `GITHUB_SERVER_URL`, and `JOB_STATUS` if `--status` is not passed. +A `push` event has no open PR to comment on -- the common case is that a PR +just merged into `development`/`main` and closed, so `GITHUB_HEAD_REF` is the +target branch, not the one that was merged (weblib-ci#14). Before giving up, +this asks Gitea which PR produced `GITHUB_SHA` +(`GET .../commits/{sha}/pull`, which resolves a closed PR from its merge +commit -- verified against both merge styles this org uses). If that also +comes up empty -- a commit that was never part of any PR -- the log itself has +nowhere to go, so a commit status is posted instead: shorter, but still +readable next to the commit, unlike silence. + Remove this once the instance is updated and logs can be read directly. """ @@ -48,6 +58,12 @@ DEFAULT_TAIL = 12000 # nix colours its output, and the raw escapes are noise in Markdown. ANSI = re.compile(r"\x1b\[[0-9;]*[A-Za-z]") +# A commit status is UI chrome (a line in a list), not a page -- measured +# against this Gitea: it accepted a 3000-char description unchanged, so +# nothing here truncates it server-side, but that does not make a wall of log +# a good idea to put there. Keep it to a snippet. +STATUS_TAIL = 500 + # The header used to hardcode "failed". Every caller guards the step with # `if: failure()`, so that was true by construction -- until an `if: always()` # probe in weblib-viewer#10 posted a failure report for a job that had passed. @@ -68,6 +84,18 @@ STATUS_PHRASES = { "skipped": "was skipped", } +# Gitea's status `state` enum is not the same vocabulary as the header +# phrases above (`pending, success, error, failure, warning, skipped`). +# A phrase this table has not heard of -- the "finished with status `X`" +# fallback -- maps to `error`: the honest reading is "something unusual +# happened", not a guess at pass or fail. +STATUS_STATE = { + "failed": "failure", + "passed": "success", + "was cancelled": "warning", + "was skipped": "skipped", +} + def status_phrase(status): """The verb for the comment header. @@ -119,8 +147,9 @@ def pull_request_for_branch(host, repo, token, branch): dispatched run that failed stayed exactly as silent as the one this script exists to fix. Looking the branch up covers those. - A push to a branch with no PR still has nowhere to comment, which is - honest: there is no thread for it. + Only finds an **open** PR. A push whose branch's PR has already merged and + closed (the common shape for `development`/`main`) is `pull_request_for_sha`'s + job, tried next. """ if not branch: return None @@ -141,6 +170,44 @@ def pull_request_for_branch(host, repo, token, branch): return None +def pull_request_for_sha(host, repo, token, sha): + """The PR that produced this commit, open or closed, or None. + + Covers what `pull_request_for_branch` cannot: a `push` to + `development`/`main` is almost always a merge landing, and by the time + the job runs, the PR that produced it is already **closed** -- so no + *open* PR has the pushed branch as its head (weblib-ci#14, found via + weblib-viewer#27). Gitea still remembers which PR a given commit came + from, independent of the PR's state: + `GET repos/{repo}/commits/{sha}/pull`. Verified against both merge + styles used in this org -- an explicit merge commit (weblib-ci#13) and a + fast-forward promotion, where the "merge commit" is just the PR branch's + own tip (weblib-fs#27) -- both resolve correctly. + + This needs the **full** 40-character sha. A short/abbreviated sha (the + kind used for display, e.g. `GITHUB_SHA[:8]`) gets exactly the same + `{"message": "pull request does not exist [id: 0, ...]"}` 404 shape as a + commit that genuinely has no PR -- so passing one silently looks + identical to "not found" rather than erroring. `GITHUB_SHA` itself is + already the full sha; this just must not be truncated before it gets + here. + """ + if not sha: + return None + url = f"{host.rstrip('/')}/api/v1/repos/{repo}/commits/{sha}/pull" + req = urllib.request.Request(url) + req.add_header("Authorization", f"token {token}") + try: + with urllib.request.urlopen(req, timeout=30) as res: + pull = json.loads(res.read() or b"null") + except (urllib.error.URLError, OSError, ValueError): + # Same reasoning as pull_request_for_branch: a 404 (no PR for this + # commit) is the expected outcome for most calls, not an error to + # surface, and a reporter that raises reports nothing. + return None + return (pull or {}).get("number") + + def tail(path, limit): with open(path, "rb") as fh: try: @@ -169,6 +236,28 @@ def comment(host, repo, number, token, body): return json.loads(res.read() or b"null") +def status_description(job, phrase, raw_text, limit=STATUS_TAIL): + """The one-line-ish body for a commit status, when there is no PR to post + the full log to. `raw_text` is plain text (no code fence, no ANSI) -- + either a log tail or `missing_log_note`'s message.""" + snippet = (raw_text or "").strip().replace("\n", " ").strip() + if len(snippet) > limit: + snippet = "…" + snippet[-limit:] + return f"{job} {phrase}: {snippet}" if snippet else f"{job} {phrase}" + + +def report_status(host, repo, sha, token, state, description, target_url): + url = f"{host.rstrip('/')}/api/v1/repos/{repo}/statuses/{sha}" + payload = {"context": "report_job_log", "state": state, + "description": description, "target_url": target_url} + req = urllib.request.Request(url, data=json.dumps(payload).encode(), + method="POST") + req.add_header("Content-Type", "application/json") + req.add_header("Authorization", f"token {token}") + with urllib.request.urlopen(req, timeout=30) as res: + return json.loads(res.read() or b"null") + + def main(argv=None): ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) @@ -192,38 +281,70 @@ def main(argv=None): print("no repo or no token; set GITHUB_REPOSITORY and GITEA_TOKEN") return 0 + full_sha = os.environ.get("GITHUB_SHA") or "" + sha = full_sha[:8] + number = args.pr or pull_request_number(os.environ.get("GITHUB_EVENT_PATH")) if number is None: branch = (os.environ.get("GITHUB_HEAD_REF") or os.environ.get("GITHUB_REF_NAME")) number = pull_request_for_branch(args.host, args.repo, args.token, branch) if number is None: - print("no pull request for this run: nothing to comment on") - return 0 + # A push to development/main has no *open* PR for the branch it + # pushed to -- that branch is the target, not the one that was + # merged and closed. Ask Gitea which PR this exact commit came from + # instead (weblib-ci#14). + number = pull_request_for_sha(args.host, args.repo, args.token, full_sha) phrase = status_phrase(args.status) - if not os.path.exists(args.log): - text = missing_log_note(args.log, phrase) + log_exists = os.path.exists(args.log) + if not log_exists: + raw = missing_log_note(args.log, phrase) else: - text = "```\n" + tail(args.log, args.tail).rstrip() + "\n```" + raw = tail(args.log, args.tail).rstrip() - sha = (os.environ.get("GITHUB_SHA") or "")[:8] - body = (f"**`{args.job}` {phrase}**{f' on `{sha}`' if sha else ''}.\n\n" - "Job logs return 500 on this Gitea, so here is the tail of the " - "build output, posted by [`report_job_log.py`]" - "(https://git.chaosbit.de/weblib/weblib-ci).\n\n" - f"{text}\n") + if number is not None: + text = ("```\n" + raw + "\n```") if log_exists else raw + body = (f"**`{args.job}` {phrase}**{f' on `{sha}`' if sha else ''}.\n\n" + "Job logs return 500 on this Gitea, so here is the tail of the " + "build output, posted by [`report_job_log.py`]" + "(https://git.chaosbit.de/weblib/weblib-ci).\n\n" + f"{text}\n") - # A failure to report a failure must not itself be silent, but it also must - # not mask the real one: the step is already `if: failure()`. + # A failure to report a failure must not itself be silent, but it also + # must not mask the real one: the step is already `if: failure()`. + try: + result = comment(args.host, args.repo, number, args.token, body) + except urllib.error.HTTPError as e: + print(f"could not comment: {e.code} " + f"{e.read()[:300].decode('utf-8', 'replace')}", file=sys.stderr) + return 0 + print(f"reported to {args.repo}#{number}: {(result or {}).get('html_url')}") + return 0 + + # No PR anywhere -- a commit that was never part of one (a direct push, + # or the lookups above genuinely found nothing). There is still no log + # endpoint to point at, but a commit status is a channel that does not + # need a thread to post into, and this repo's Actions already post + # statuses that people read. + if not full_sha: + print("no pull request and no commit sha for this run: nothing to " + "report") + return 0 + + state = STATUS_STATE.get(phrase, "error") + description = status_description(args.job, phrase, raw) + target_url = f"{args.host.rstrip('/')}/{args.repo}/commit/{full_sha}" try: - result = comment(args.host, args.repo, number, args.token, body) + report_status(args.host, args.repo, full_sha, args.token, state, + description, target_url) except urllib.error.HTTPError as e: - print(f"could not comment: {e.code} " + print(f"could not report status: {e.code} " f"{e.read()[:300].decode('utf-8', 'replace')}", file=sys.stderr) return 0 - print(f"reported to {args.repo}#{number}: {(result or {}).get('html_url')}") + print(f"reported to {args.repo}@{sha} as a commit status " + "(no pull request found)") return 0 diff --git a/test_report_job_log.py b/test_report_job_log.py index 4334e32..b2b6006 100755 --- a/test_report_job_log.py +++ b/test_report_job_log.py @@ -42,10 +42,16 @@ ACTIONS_ENV = [ class FakeGitea(BaseHTTPRequestHandler): - """Just enough of the API: list open PRs, and accept a comment.""" + """Just enough of the API: list open PRs, resolve a commit's merged PR, + accept a comment, and accept a commit status.""" comments = [] # class-level; reset per test + statuses = [] pulls = [] + # sha -> pull dict, mirroring GET .../commits/{sha}/pull. A sha absent + # from this map 404s, the same shape Gitea returns for a commit that was + # never part of any PR. + pull_for_sha = {} def _send(self, code, payload): raw = json.dumps(payload).encode() @@ -56,6 +62,14 @@ class FakeGitea(BaseHTTPRequestHandler): self.wfile.write(raw) def do_GET(self): + if "/commits/" in self.path and self.path.endswith("/pull"): + sha = self.path.split("/commits/", 1)[1].rsplit("/pull", 1)[0] + pull = type(self).pull_for_sha.get(sha) + if pull is None: + return self._send( + 404, {"message": "pull request does not exist " + "[id: 0, issue_id: 0]"}) + return self._send(200, pull) if "/pulls" in self.path: return self._send(200, type(self).pulls) self._send(404, {"message": "no"}) @@ -63,6 +77,16 @@ class FakeGitea(BaseHTTPRequestHandler): def do_POST(self): length = int(self.headers.get("Content-Length") or 0) payload = json.loads(self.rfile.read(length) or b"{}") + if "/statuses/" in self.path: + type(self).statuses.append({ + "path": self.path, + "auth": self.headers.get("Authorization"), + "context": payload.get("context"), + "state": payload.get("state"), + "description": payload.get("description", ""), + "target_url": payload.get("target_url"), + }) + return self._send(201, {"id": 1, "state": payload.get("state")}) type(self).comments.append({ "path": self.path, "auth": self.headers.get("Authorization"), @@ -79,7 +103,9 @@ class ServerTestCase(unittest.TestCase): def setUp(self): FakeGitea.comments = [] + FakeGitea.statuses = [] FakeGitea.pulls = [] + FakeGitea.pull_for_sha = {} self.server = HTTPServer(("127.0.0.1", 0), FakeGitea) self.host = "http://127.0.0.1:%d" % self.server.server_address[1] # `shutdown()` only takes effect on the next poll, so the default @@ -115,6 +141,10 @@ class ServerTestCase(unittest.TestCase): """ return FakeGitea.comments + def posted_statuses(self): + """Same idea as `posted()`, for the commit-status fallback.""" + return FakeGitea.statuses + def run_main(self, *argv): """The entry point in-process, so a traceback is readable.""" out = io.StringIO() @@ -293,6 +323,130 @@ class TestEndToEnd(ServerTestCase): self.assertIn("--status", proc.stdout) +# A commit that landed by merging a now-closed PR into development/main -- +# weblib-ci#14's actual failure mode. A real 40-char sha, because +# pull_request_for_sha silently treats a truncated one the same as "no PR". +PUSH_SHA = "c9d6db67d9c20f8ab133f4fb5fea359e9c474274" + + +class TestPushWithNoOpenPR(ServerTestCase): + """A `push` event: no event payload, no GITHUB_HEAD_REF, and the pushed + branch (GITHUB_REF_NAME) is the *target* of a merge, not a PR head -- the + exact shape of weblib-ci#14 (weblib-viewer run 1769, PR#27 merged into + `development`).""" + + def _run(self, *argv, env=None): + full = dict(os.environ) + full.pop("GITEA_HOST", None) + full.update({ + "GITHUB_REPOSITORY": "weblib/weblib-ci", + "GITHUB_SERVER_URL": self.host, + "GITEA_TOKEN": "t0ken", + "GITHUB_JOB": "publish", + "GITHUB_SHA": PUSH_SHA, + "GITHUB_REF_NAME": "development", + }) + full.update(env or {}) + return subprocess.run([sys.executable, SCRIPT, self.log, *argv], + capture_output=True, text=True, env=full, + cwd=HERE) + + def test_falls_back_to_the_pr_the_push_merged(self): + """No open PR has `development` as its head -- it never does -- but + the merge commit itself resolves to the PR that produced it, even + though that PR is now closed.""" + FakeGitea.pulls = [] + FakeGitea.pull_for_sha = {PUSH_SHA: {"number": 27, "state": "closed"}} + + proc = self._run() + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("reported to weblib/weblib-ci#27", proc.stdout) + + posted = self.posted() + self.assertEqual(len(posted), 1, posted) + self.assertEqual(posted[0]["path"], + "/api/v1/repos/weblib/weblib-ci/issues/27/comments") + self.assertIn("DISTINCTIVE-LINE", posted[0]["body"]) + self.assertEqual(self.posted_statuses(), [], + "a resolvable PR must not also get a status") + + def test_branch_lookup_still_wins_when_it_applies(self): + """Ordering: an *open* PR whose head matches the branch is found + first, so the (potentially stale) sha lookup is never even tried.""" + FakeGitea.pulls = [{"number": 5, "head": {"ref": "development"}}] + FakeGitea.pull_for_sha = {PUSH_SHA: {"number": 999}} + + proc = self._run() + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("reported to weblib/weblib-ci#5", proc.stdout) + self.assertNotIn("999", proc.stdout) + + def test_no_pr_anywhere_reports_a_commit_status(self): + """Not even the sha lookup finds anything -- a commit that was never + part of any PR. This must still report *somewhere*, not silently + exit 0 the way #14 did.""" + FakeGitea.pulls = [] + FakeGitea.pull_for_sha = {} + + proc = self._run() + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn(f"reported to weblib/weblib-ci@{PUSH_SHA[:8]}", + proc.stdout) + self.assertEqual(self.posted(), [], "no PR exists to comment on") + + statuses = self.posted_statuses() + self.assertEqual(len(statuses), 1, statuses) + s = statuses[0] + self.assertEqual(s["path"], + f"/api/v1/repos/weblib/weblib-ci/statuses/{PUSH_SHA}") + self.assertEqual(s["auth"], "token t0ken") + self.assertEqual(s["state"], "failure") + self.assertIn("publish", s["description"]) + self.assertIn("DISTINCTIVE-LINE", s["description"]) + self.assertIn(PUSH_SHA, s["target_url"]) + + def test_commit_status_state_follows_the_reported_outcome(self): + FakeGitea.pulls = [] + FakeGitea.pull_for_sha = {} + + for status, want_state in [("passed", "success"), + ("cancelled", "warning"), + ("skipped", "skipped"), + ("neutral", "error")]: + with self.subTest(status=status): + FakeGitea.statuses = [] + proc = self._run("--status", status) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertEqual(self.posted_statuses()[0]["state"], want_state) + + def test_no_sha_and_no_pr_does_not_crash_or_post_anything(self): + """The one case with nowhere at all to report: still exits 0 and + says so, rather than raising or posting a status with an empty sha + in the URL.""" + FakeGitea.pulls = [] + FakeGitea.pull_for_sha = {} + + proc = self._run(env={"GITHUB_SHA": "", "GITHUB_REF_NAME": "development"}) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("nothing to report", proc.stdout) + self.assertEqual(self.posted(), []) + self.assertEqual(self.posted_statuses(), []) + + def test_existing_pr_event_path_is_unaffected(self): + """The ordinary pull_request-event path -- unrelated to any of the + push-triggered fallback -- still goes straight to the comment, and + never touches the sha or status endpoints.""" + path = os.path.join(self.mkdtemp(), "event.json") + with open(path, "w") as fh: + json.dump({"pull_request": {"number": 11}}, fh) + FakeGitea.pull_for_sha = {PUSH_SHA: {"number": 999}} # must be ignored + + proc = self._run(env={"GITHUB_EVENT_PATH": path}) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("reported to weblib/weblib-ci#11", proc.stdout) + self.assertEqual(self.posted_statuses(), []) + + class TestStatusPhrase(unittest.TestCase): """The unit under the header, without a server."""