From 3c87a034a6cb4faf61a05607608d723af25ad0f9 Mon Sep 17 00:00:00 2001 From: Jesper Terkelsen Date: Tue, 13 Oct 2026 17:50:53 +0200 Subject: [PATCH] feat(actions): keep one arc-job-report comment per PR with a section per workflow monta-app/server runs its ARC jobs in several workflows (Unit tests, Integration tests, Static checks, Smoke tests). With one section per comment, each workflow's run would overwrite the others' job table and mix their run histories. The comment now has a section per workflow, marked with ""; a run re-renders only its own section, and the 50-run history is split evenly between sections to stay under GitHub's comment size limit. A comment from before sections is adopted as the reporting workflow's section, keeping its history. A run whose commit has no PR now exits 0 with a message instead of failing: push-triggered workflows on branches without a PR are normal. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/actions/arc-job-report/README.md | 18 ++++++--- .../actions/arc-job-report/arc_job_report.py | 26 ++++++++----- .../arc-job-report/test_arc_job_report.py | 38 +++++++++++++++++-- CLAUDE.md | 5 ++- 4 files changed, 67 insertions(+), 20 deletions(-) diff --git a/.github/actions/arc-job-report/README.md b/.github/actions/arc-job-report/README.md index 258494a..2871135 100644 --- a/.github/actions/arc-job-report/README.md +++ b/.github/actions/arc-job-report/README.md @@ -2,13 +2,14 @@ Keeps **one comment per PR** that shows how each job of a workflow run used its self-hosted ARC runner, which jobs would fit a different runner size, and how the -run's wall time changes from commit to commit. +run's wall time changes from commit to commit. Each reported workflow gets its own +section in that comment, and a run only updates its own workflow's section. -Collapsed, the comment is a single line: +Collapsed, each section is a single line: -> ▶ ✅ **4m 33s** (▲ 18s vs previous run) · 🏃 ARC runners · [run #6228](#) · 315e3c4 · 💡 2 runner changes +> ▶ Unit tests · ✅ **4m 33s** (▲ 18s vs previous run) · 🏃 ARC runners · [run #6228](#) · 315e3c4 · 💡 2 runner changes -Expanded, it has: +Expanded, a section has: - **Jobs:** each job's duration, with the job name linking to its pod on the [ARC Job Resources dashboard](https://montaapp.grafana.net/d/arc-job-resources). @@ -23,10 +24,12 @@ Expanded, it has: - **Run history:** one row per run (commit, ✅/❌, wall time, change vs the previous run, slowest job), so speed changes can be traced to commits. The history is stored as JSON in a hidden HTML comment and re-rendered on every - run, keeping the last 50 runs. + run, keeping the last 50 runs, split evenly between the sections. The comment is edited in place on every run. Only a bot-authored comment is updated, so a comment someone posted by running the script locally is left alone. +A comment from before sections existed is adopted as the reporting workflow's +section, history included. ## Usage @@ -62,6 +65,11 @@ jobs: No checkout is needed: the action ships its own scripts. Jobs that did not run on an ARC runner (`arc-*` / `self-hosted-*`) are skipped. +To report several workflows, list them all in `workflows:`; they share the PR's one +comment. If they run on `push` rather than `pull_request` (as in monta-app/server), +drop the `event == 'pull_request'` condition: the report finds the PR from the +commit, and a run whose commit has no PR is skipped without failing. + | Input | Required | Description | |---|---|---| | `github-token` | yes | Reads the run's jobs and writes the comment. Needs `actions: read` and `pull-requests: write`. | diff --git a/.github/actions/arc-job-report/arc_job_report.py b/.github/actions/arc-job-report/arc_job_report.py index a997441..62fd8da 100644 --- a/.github/actions/arc-job-report/arc_job_report.py +++ b/.github/actions/arc-job-report/arc_job_report.py @@ -6,8 +6,8 @@ Needs `gh` authenticated (GH_TOKEN) and GRAFANA_TOKEN, or `gcx` for local runs; see job_resources.py. -The PR gets one comment: the latest run's per-job table, plus a history row per run so -speed changes can be traced to commits. Only a bot-authored comment is updated, so run it +The PR gets one comment with a section per workflow: the workflow's latest run's per-job +table, plus a history row per run so speed changes can be traced to commits. Only a bot-authored comment is updated, so run it locally with --dry-run, which prints the comment instead of posting it. """ import json @@ -20,6 +20,7 @@ MARKER = "" HISTORY = re.compile(r"", re.S) +SECTION = re.compile(r"\n(.*?)(?=\n\n{s}" for w, s in sorted(sections.items()))]) + + def main(): args = [a for a in sys.argv[1:] if not a.startswith("--")] if not args: @@ -181,13 +188,14 @@ def main(): run = json.loads(sh("gh", "api", f"repos/{repo}/actions/runs/{run_id}")) prs = run["pull_requests"] or json.loads(sh("gh", "api", f"repos/{repo}/commits/{run['head_sha']}/pulls")) if not prs: - sys.exit(f"no pull request found for run {run_id}") + print(f"no pull request found for run {run_id}, nothing to report") + return pr = prs[0]["number"] results = analyse_run(args[0]) comments = json.loads(sh("gh", "api", "--paginate", "--slurp", f"repos/{repo}/issues/{pr}/comments")) existing = next((c for page in reversed(comments) for c in reversed(page) if MARKER in c["body"] and c["user"]["type"] == "Bot"), None) - body = render(run, results, existing and existing["body"]) + body = update(existing and existing["body"], run["name"], run, results) if "--dry-run" in sys.argv: print(body) elif existing: diff --git a/.github/actions/arc-job-report/test_arc_job_report.py b/.github/actions/arc-job-report/test_arc_job_report.py index 18ee38c..ae2c1e3 100644 --- a/.github/actions/arc-job-report/test_arc_job_report.py +++ b/.github/actions/arc-job-report/test_arc_job_report.py @@ -4,12 +4,12 @@ sys.path.insert(0, str(Path(__file__).parent)) -from arc_job_report import render # noqa: E402 +from arc_job_report import MARKER, render, update # noqa: E402 from job_resources import history, suggest # noqa: E402 -def run(number, attempt=1, ended="2026-09-28T11:07:46Z", conclusion="success"): - return {"run_number": number, "run_attempt": attempt, "html_url": f"https://example/runs/{number}", +def run(number, attempt=1, ended="2026-09-28T11:07:46Z", conclusion="success", workflow="Unit tests"): + return {"name": workflow, "run_number": number, "run_attempt": attempt, "html_url": f"https://example/runs/{number}", "head_sha": f"{number}abcdef", "conclusion": conclusion, "run_started_at": "2026-09-28T11:00:00Z", "updated_at": ended} @@ -37,7 +37,7 @@ def test_history_keeps_earlier_runs_sorted_replaces_a_reported_run_and_shows_the rows = history_rows(body) self.assertEqual([r.split("]")[0] for r in rows], ["| [#10", "| [#12"]) self.assertIn("| ✅ | 8m 00s | ▲ 14s | Compile (1m 20s) |", rows[1]) - self.assertIn("✅ 8m 00s (▲ 14s vs previous run)", body) + self.assertIn("Unit tests · ✅ 8m 00s (▲ 14s vs previous run)", body) def test_merges_shards_into_one_row_with_a_link_per_shard_and_the_slowest_in_bold(self): body = render(run(1), [job("Tests - shard 1", 110), job("Tests - shard 0", 130), job("Tests - shard 2", 90)], None) @@ -109,6 +109,36 @@ def test_omits_the_suggestion_table_when_every_runner_fits(self): self.assertNotIn("💡", body) +class UpdateTest(unittest.TestCase): + def test_keeps_one_section_per_workflow_and_replaces_only_the_reporting_one(self): + body = update(None, "Unit tests", run(5), [job("Compile", 100)]) + body = update(body, "Integration tests", run(9, workflow="Integration tests"), [job("Suite", 200)]) + body = update(body, "Unit tests", run(6), [job("Compile", 80)]) + + self.assertTrue(body.startswith(MARKER + "\n\n\n
")) + self.assertEqual(update(body, "Unit tests", run(6), [job("Compile", 80)]), body) + self.assertEqual(body.count("")[1].split("| Run |")[0]) + self.assertEqual([r.split("]")[0] for r in history_rows(body)], ["| [#9", "| [#5", "| [#6"]) + + def test_adopts_a_comment_from_before_sections_as_the_reporting_workflow(self): + legacy = MARKER + "\n" + render(run(5), [job("Compile", 100)], None) + body = update(legacy, "Unit tests", run(6), [job("Compile", 80)]) + + self.assertEqual(body.count(""), 1) + self.assertEqual([r.split("]")[0] for r in history_rows(body)], ["| [#5", "| [#6"]) + + def test_splits_the_run_history_between_the_workflows(self): + body = None + for n in range(1, 40): + body = update(body, "Unit tests", run(n), [job("Compile", 100)]) + body = update(body, "Integration tests", run(n, workflow="Integration tests"), [job("Suite", 100)]) + + self.assertEqual(len(history_rows(body)), 50) + + def sized(peak_mem_gib=3.0, cpu_p50=1.0, cpu_peak=2.0, runs=40, oom=0, runner=POD): return suggest(runner, "arm64", peak_mem_gib, cpu_p50, cpu_peak, runs, oom) diff --git a/CLAUDE.md b/CLAUDE.md index 6791a55..8267c6b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,14 +2,15 @@ ## Last Documentation Update - **Date**: 2026-10-13 -- **Latest SHA**: 79f05d3 (check for newer commits) -- **Changes**: `arc-job-report` merges ` (shard N)` / `(shard-N)` shards into one row in both tables +- **Latest SHA**: 821e53e (check for newer commits) +- **Changes**: `arc-job-report` keeps one comment per PR with a section per workflow ## Recent Changes (2026-10-13) 1. **`argocd-resource-name` on the v2 deploy workflows**: `deploy-generic-v2.yml` and `deploy-kotlin-v2.yml` accept `argocd-resource-name` and pass it through `component-deploy-v2.yml` to `argocd-wait-sync`'s existing `resource-name` (kind stays `Deployment`). The wait then requires the expected revision, `Synced`, and that Deployment healthy, and ignores the app's aggregate health. Without it, any Degraded resource in the app fails the deploy on the first poll: service-monta-factory run 38388140776 failed with its Deployment `Healthy` because a CronJob's last Job had failed. Empty (the default) keeps tracking the whole app. 1. **`arc-job-report` reads `mimir-internal` at 15 s**: ARC's cAdvisor and kube-state-metrics series stopped arriving in `grafanacloud-prom` around 2026-10-01, so every job came back `no metrics` and no runner was ever suggested. `job_resources.py` queries `mimir-internal` (uid `ffzoywkuhm5tsd`) at a 15 s step with `rate[1m]`; the old 60 s / `rate[2m]` reported 2-minute averages and hid CPU bursts (monorepo Lint p90 peak 1.8 → 3.3 cores). 1. **`arc-job-report` sizes runners from the job's last 7 days**: one run is not safe to size on, because cache hits change a job's peak memory a lot (monorepo Typecheck: 1.5 GB typical, 13.1 GB worst). The history comes from the `arc:job_memory_working_set_bytes:max1m` and `arc:job_cpu_usage_cores:rate1m` recording rules in monta-app/terraform, which join each runner pod to its GitHub job (`repository`, `job_name`, `workflow_run_id`) once a minute. Doing that join at read time over 7 days OOM-killed the mimir-internal query-frontends, so the report must only read the recorded series. Runners have a memory limit but only a CPU request, so memory is a hard floor (1.3 × the 7-day peak; an OOM kill always moves up a size) and CPU is a labelled trade-off: ⬆️ *CPU-bound, faster* when the median is ≥ 85% of the runner's vCPU; a smaller size must cover the median and is *fits* or *trial: may be slower* depending on the peak. Smaller sizes need 5 runs in the window; there is no backfill, so they start a week after the rules are applied. The old 2-minute cut-off is gone, since the history covers short jobs. The local path uses `gcx api` against the same datasource-proxy URL as CI, because `gcx metrics query` silently widens the step to about 100 points. 1. **`arc-job-report` groups `(shard N)` and `(shard-N)` shards**: shards were only recognised as ` - shard N` (service-ocpi; server uses `unit-tests (shard-N)`), and `name()` strips a `(…)` suffix before that check, so monorepo-typescript's `Test (shard N)` jobs collapsed into one jobs-table row together with the separate `Test` gate job (linking only the slowest shard), and got one suggestion row each ("💡 11 runner changes", 7 of them identical Test rows). Both formats now group into a `Test · shards 1 2 …` row in both tables; in the suggestion table shards are grouped per change and reason, showing the highest memory and CPU and the fewest runs, and the 💡 count is the number of rows. Jobs that aren't shards are grouped by their full name, so two jobs differing only in a `(…)` suffix keep separate rows. +1. **`arc-job-report` keeps one comment per PR with a section per workflow**: monta-app/server runs its ARC jobs in several workflows (`Unit tests`, `Integration tests`, `Static checks`, `Smoke tests`), and with a single section each workflow's run would have overwritten the others' table and mixed their run histories. Each section starts with ``; a run re-renders only its own, and the 50-run history is split evenly between sections to stay under GitHub's comment limit. A comment from before sections is adopted as the reporting workflow's section, history included. A run whose commit has no PR now prints a message and exits 0 instead of failing, since push-triggered workflows on a branch without a PR are normal (monorepo-typescript had a red run from it). ## Recent Changes (2026-10-07) 1. **Terraform plans are stored beside the state, not as Actions artifacts**: `terraform-stack.yml` writes the saved plan to `s3:///tfplans//.tfplan`, read from the initialised backend in `.terraform/terraform.tfstate`, and the apply downloads and then deletes it. A saved plan is a zip of the full prior state (`tfstate`, `tfstate-prev`) plus every variable value, `sensitive` or not, and Actions artifacts are downloadable by anyone with read access to the repository — the whole org. Verified locally: a `sensitive` data-source value was absent from the text plan and present in `terraform show -json tfplan`. The shared `tfplans/` prefix is what lets one bucket lifecycle rule expire unapplied plans and the noncurrent versions left by the post-apply delete (S3 lifecycle filters only match a literal key prefix); the cost is that roles scoped to their own state prefix need `tfplans//*` added. Stacks must use the `s3` backend; anything else fails with `Unsupported backend`. `plan-retention-days` and the `Slug the stack path` step are gone, and callers no longer need `actions: write`. The runner needs the AWS CLI. Callers pinned to an older SHA keep uploading artifacts until they bump, and a PR planned under the old pin finds no plan after the bump, so it needs a fresh plan or a `recover`.