You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@magpie.apache.org by po...@apache.org on 2026/10/24 11:53:42 UTC
(magpie-site) branch main updated: Run previews and browser suites only for PRs that change the site (#248)
This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/magpie-site.git
The following commit(s) were added to refs/heads/main by this push:
new 4abc214 Run previews and browser suites only for PRs that change the site (#248)
4abc214 is described below
commit 4abc214f8c4f60dc8100b50ee8501998dd0fc834
Author: Jarek Potiuk <ja...@potiuk.com>
AuthorDate: Sat Oct 24 13:53:36 2026 +0200
Run previews and browser suites only for PRs that change the site (#248)
* Offer PR previews only when the site's sources or dependencies change
Bump website-comment to v1.1.0 and use its new inputs: preview-artifact's
paths skips the upload for a PR that changes none of the site's sources,
docs or dependencies, and preview-publish's howto: with-preview-artifact
posts the explainer only where a preview was uploaded, and tells an armed
PR without one why there is no preview.
Generated-by: Claude Opus 5
* Run the browser suites only when a PR changes the site or the suites
A changes job decides from the PR's changed files, against one list of the
site's paths: a preview when the PR changes the site, and the browser suites
when it changes the site or what the suites are made of. A PR touching only
workflows or repository settings now skips the 22 browser jobs. The preview
upload uses the same decision, so the two cannot drift; build waits for it,
so a failed decision fails the PR rather than skipping its tests. Pushes,
the weekly run and dispatches run everything.
Generated-by: Claude Opus 5
---
.github/browser-test-paths.txt | 10 +++++
.github/site-paths.txt | 14 +++++++
.github/workflows/build.yml | 63 ++++++++++++++++++++++------
.github/workflows/preview-publish.yml | 5 ++-
scripts/pr-changes.mjs | 78 +++++++++++++++++++++++++++++++++++
scripts/pr-changes.test.mjs | 66 +++++++++++++++++++++++++++++
scripts/preview-workflows.test.mjs | 24 +++++++++++
7 files changed, 247 insertions(+), 13 deletions(-)
diff --git a/.github/browser-test-paths.txt b/.github/browser-test-paths.txt
new file mode 100644
index 0000000..f71c3c5
--- /dev/null
+++ b/.github/browser-test-paths.txt
@@ -0,0 +1,10 @@
+# Beyond site-paths.txt, what the browser suites are made of: a pull request
+# changing any of these runs them, though it gets no preview. Read by
+# scripts/pr-changes.mjs; same format as site-paths.txt.
+tests/
+playwright.config.mjs
+playwright.dev.config.mjs
+.github/workflows/browser-tests.yml
+.github/actions/playwright-chromium/
+.github/site-paths.txt
+.github/browser-test-paths.txt
diff --git a/.github/site-paths.txt b/.github/site-paths.txt
new file mode 100644
index 0000000..0d9a07b
--- /dev/null
+++ b/.github/site-paths.txt
@@ -0,0 +1,14 @@
+# The paths the site is built from. A pull request that changes none of them
+# gets no preview, and runs the browser suites only if it changes one of
+# browser-test-paths.txt. Read by scripts/pr-changes.mjs.
+#
+# One path per line: `dir/` is everything under dir, anything else is that one
+# file. Every path must exist (scripts/pr-changes.test.mjs checks it).
+src/
+public/
+scripts/
+docs/
+astro.config.mjs
+tsconfig.json
+package.json
+package-lock.json
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index ffde5ca..66b7c3e 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -72,21 +72,55 @@ jobs:
uv tool install prek
prek run --all-files --color always
- # The browser suites, as their own workflow so their 17 matrix jobs are
- # grouped under one "browser" entry in the checks list. They start at once
- # rather than after lint: they are the longest part of the run, and `build`
- # still waits for both.
+ # What a pull request needs, from the files it changes: a preview when it
+ # changes the site (.github/site-paths.txt), and the browser suites when it
+ # changes the site or the suites (.github/browser-test-paths.txt). One
+ # decision for both, so they cannot drift apart. Other events run everything.
+ changes:
+ if: github.event_name == 'pull_request'
+ runs-on: ubuntu-slim
+ timeout-minutes: 5
+ permissions:
+ contents: read
+ pull-requests: read # list the PR's changed files
+ outputs:
+ site: ${{ steps.decide.outputs.site }}
+ browser: ${{ steps.decide.outputs.browser }}
+ steps:
+ - name: Checkout
+ uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+ with:
+ persist-credentials: false
+ sparse-checkout: |
+ .github
+ scripts
+
+ - name: Decide what this pull request needs
+ id: decide
+ env:
+ GITHUB_TOKEN: ${{ github.token }}
+ PR_NUMBER: ${{ github.event.pull_request.number }}
+ run: node scripts/pr-changes.mjs
+
+ # The browser suites, as their own workflow so their 22 matrix jobs are
+ # grouped under one "browser" entry in the checks list. They start once
+ # `changes` has decided, not after lint: they are the longest part of the
+ # run, and `build` still waits for both. Skipped on a pull request that
+ # changes neither the site nor the suites; the weekly run covers main.
browser:
- if: github.event_name != 'push'
+ needs: changes
+ if: ${{ !cancelled() && github.event_name != 'push' && (github.event_name != 'pull_request' || needs.changes.outputs.browser == 'true') }}
permissions:
contents: read
uses: $/.github/workflows/browser-tests.yml
build:
- needs: [lint, browser]
- # Run when lint and browser passed, or were skipped on a push. A shard that
- # hits its timeout ends "cancelled", not "failure", so !failure() alone
- # would let the build through.
+ # `changes` is a need of its own: if it fails, browser is skipped rather
+ # than failed, and only this keeps a broken decision from passing the PR.
+ needs: [changes, lint, browser]
+ # Run when lint and browser passed, or were skipped. A shard that hits its
+ # timeout ends "cancelled", not "failure", so !failure() alone would let
+ # the build through.
if: ${{ !cancelled() && !contains(needs.*.result, 'failure') && !contains(needs.*.result, 'cancelled') }}
runs-on: ubuntu-slim
timeout-minutes: 10
@@ -149,7 +183,7 @@ jobs:
# cannot be linked here; the issue carries only the page for those.
- name: Add the review overlay to the published site
if: github.ref == 'refs/heads/main' && env.MAIN_REVIEW_OVERLAY == 'true'
- uses: apache/infrastructure-actions/website-comment/preview-main-overlay@65a538352531210f043ebee7cb6f87d3773a73e2 # preview-main-overlay/v1.0.0
+ uses: apache/infrastructure-actions/website-comment/preview-main-overlay@9ef89ae3b50c5a404b06e200efaa6143dad61d83 # preview-main-overlay/v1.1.0
with:
path: dist
generated: src/content/docs/
@@ -165,9 +199,14 @@ jobs:
# diff here rather than in the privileged publisher, which never reads
# pull-request content, and uploads dist (dotfiles included: .htaccess
# carries the 404 handler) as the preview-site artifact.
+ #
+ # Only when the PR changes what the site is built from, as `changes`
+ # decided: one that touches only CI, tests or repository settings has
+ # nothing new to preview, and preview-publish.yml then posts no
+ # explainer for it.
- name: Upload the preview
- if: github.event_name == 'pull_request'
- uses: apache/infrastructure-actions/website-comment/preview-artifact@65a538352531210f043ebee7cb6f87d3773a73e2 # preview-artifact/v1.0.0
+ if: github.event_name == 'pull_request' && needs.changes.outputs.site == 'true'
+ uses: apache/infrastructure-actions/website-comment/preview-artifact@9ef89ae3b50c5a404b06e200efaa6143dad61d83 # preview-artifact/v1.1.0
with:
path: dist
diff --git a/.github/workflows/preview-publish.yml b/.github/workflows/preview-publish.yml
index 194e1be..242230f 100644
--- a/.github/workflows/preview-publish.yml
+++ b/.github/workflows/preview-publish.yml
@@ -95,11 +95,14 @@ jobs:
cancel-in-progress: false
steps:
- name: Publish and retire previews
- uses: apache/infrastructure-actions/website-comment/preview-publish@65a538352531210f043ebee7cb6f87d3773a73e2 # preview-publish/v1.0.0
+ uses: apache/infrastructure-actions/website-comment/preview-publish@9ef89ae3b50c5a404b06e200efaa6143dad61d83 # preview-publish/v1.1.0
with:
site-name: magpie # magpie-pr<N>.staged.apache.org
build-workflow: build.yml
pr: ${{ inputs.pr }}
+ # Explain previews only on PRs whose build uploaded one: build.yml
+ # skips the upload when a PR changes none of the site's paths.
+ howto: with-preview-artifact
# Also delete this repository's head branches once all their pull
# requests have closed, as the in-repo publisher always did.
reap-head-branches: "true"
diff --git a/scripts/pr-changes.mjs b/scripts/pr-changes.mjs
new file mode 100644
index 0000000..5847ca3
--- /dev/null
+++ b/scripts/pr-changes.mjs
@@ -0,0 +1,78 @@
+// Decide, from a pull request's changed files, what its CI run needs.
+//
+// `site`: it changes what the site is built from (.github/site-paths.txt), so
+// it gets a preview. `browser`: it changes the site or what the browser suites
+// are made of (.github/browser-test-paths.txt), so the suites run. A pull
+// request touching only workflows, repository settings or docs for agents
+// gets neither. Runs in build.yml's unprivileged `changes` job, and writes
+// both answers to $GITHUB_OUTPUT.
+import { appendFile, readFile } from "node:fs/promises";
+import { fileURLToPath } from "node:url";
+
+const ROOT = new URL("../", import.meta.url);
+
+/** The entries of a paths file: one per line, without blanks or `#` comments. */
+export function parsePaths(text) {
+ return text
+ .split("\n")
+ .map((line) => line.trim())
+ .filter((line) => line && !line.startsWith("#"));
+}
+
+/** Whether a repository path is one of `entries`: `dir/` covers everything under it. */
+export function matches(file, entries) {
+ return entries.some((entry) => (entry.endsWith("/") ? file.startsWith(entry) : file === entry));
+}
+
+/**
+ * Whether any changed file is one of `entries`. A rename counts under its old
+ * name too: moving a file out of the site changes the site.
+ */
+export function touches(files, entries) {
+ return files.some((f) => [f?.filename, f?.previous_filename].some((name) => typeof name === "string" && matches(name, entries)));
+}
+
+/**
+ * Both decisions for these changed files. `files` is null when they could not
+ * be listed, and then everything runs: a preview or a test run nobody needed
+ * costs less than a site change that went untested.
+ */
+export function decide(files, { site, tests }) {
+ if (files === null) return { site: true, browser: true };
+ const changesSite = touches(files, site);
+ return { site: changesSite, browser: changesSite || touches(files, tests) };
+}
+
+export async function readPaths(name) {
+ return parsePaths(await readFile(new URL(`.github/${name}`, ROOT), "utf8"));
+}
+
+async function listFiles({ repo, pr, token }) {
+ const files = [];
+ for (let page = 1; ; page += 1) {
+ const res = await fetch(`https://api.github.com/repos/${repo}/pulls/${pr}/files?per_page=100&page=${page}`, {
+ headers: { authorization: `Bearer ${token}`, accept: "application/vnd.github+json", "x-github-api-version": "2022-11-28" },
+ });
+ if (!res.ok) throw new Error(`listing the pull request's files failed with ${res.status}`);
+ const batch = await res.json();
+ files.push(...batch);
+ if (batch.length < 100) return files;
+ }
+}
+
+if (process.argv[1] === fileURLToPath(import.meta.url)) {
+ const { GITHUB_REPOSITORY: repo, GITHUB_TOKEN: token, PR_NUMBER: pr, GITHUB_OUTPUT: out } = process.env;
+ if (!/^\d+$/.test(String(pr)) || !out) {
+ console.error("PR_NUMBER and GITHUB_OUTPUT are required");
+ process.exit(1);
+ }
+ let files = null;
+ try {
+ files = await listFiles({ repo, pr, token });
+ } catch (err) {
+ console.error(`::warning::${err.message}; running everything`);
+ }
+ const result = decide(files, { site: await readPaths("site-paths.txt"), tests: await readPaths("browser-test-paths.txt") });
+ console.log(`preview: ${result.site ? "yes" : "no"}; browser suites: ${result.browser ? "yes" : "no"}`);
+ await appendFile(out, `site=${result.site}\nbrowser=${result.browser}\n`);
+}
diff --git a/scripts/pr-changes.test.mjs b/scripts/pr-changes.test.mjs
new file mode 100644
index 0000000..adc5961
--- /dev/null
+++ b/scripts/pr-changes.test.mjs
@@ -0,0 +1,66 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { existsSync, statSync } from "node:fs";
+import { decide, matches, parsePaths, readPaths, touches } from "./pr-changes.mjs";
+
+const files = (...names) => names.map((filename) => ({ filename }));
+const lists = { site: ["src/", "package.json"], tests: ["tests/", "playwright.config.mjs"] };
+
+test("paths files hold one entry per line, without blanks or comments", () => {
+ assert.deepEqual(parsePaths("# about\n\nsrc/\n package.json \n"), ["src/", "package.json"]);
+});
+
+test("a directory entry covers everything under it, a file entry only itself", () => {
+ assert.ok(matches("src/pages/index.astro", ["src/"]));
+ assert.ok(!matches("srcs/x", ["src/"]));
+ assert.ok(!matches("docs/src/x", ["src/"]));
+ assert.ok(matches("package.json", ["package.json"]));
+ assert.ok(!matches("scripts/package.json", ["package.json"]));
+ assert.ok(!matches("package.json.bak", ["package.json"]));
+});
+
+test("a rename counts under its old name too", () => {
+ assert.ok(touches([{ filename: "attic/a.astro", previous_filename: "src/a.astro" }], ["src/"]));
+});
+
+test("a site change gets a preview and runs the browser suites", () => {
+ assert.deepEqual(decide(files(".asf.yaml", "src/a.astro"), lists), { site: true, browser: true });
+});
+
+test("a test-only change runs the browser suites without a preview", () => {
+ assert.deepEqual(decide(files("tests/browser/site.spec.mjs"), lists), { site: false, browser: true });
+});
+
+test("a change to neither gets neither", () => {
+ assert.deepEqual(decide(files(".github/workflows/build.yml", ".asf.yaml", "AGENTS.md"), lists), { site: false, browser: false });
+});
+
+test("when the files could not be listed, everything runs", () => {
+ assert.deepEqual(decide(null, lists), { site: true, browser: true });
+});
+
+test("every listed path exists, and directories are written as dir/", async () => {
+ // A path renamed or deleted without updating the list would silently stop
+ // previewing, or testing, whatever it named.
+ for (const name of ["site-paths.txt", "browser-test-paths.txt"]) {
+ const entries = await readPaths(name);
+ assert.ok(entries.length > 0, `${name} lists something`);
+ for (const entry of entries) {
+ const path = new URL(`../${entry}`, import.meta.url);
+ assert.ok(existsSync(path), `${name}: ${entry} exists`);
+ assert.equal(statSync(path).isDirectory(), entry.endsWith("/"), `${name}: ${entry} is written as ${statSync(path).isDirectory() ? "a directory" : "a file"}`);
+ }
+ }
+});
+
+test("the site, the suites and their wiring are all covered", async () => {
+ const site = await readPaths("site-paths.txt");
+ const tests = await readPaths("browser-test-paths.txt");
+ for (const file of ["src/pages/index.astro", "public/favicon.svg", "package-lock.json", "astro.config.mjs", "docs/designs/x.md"]) {
+ assert.ok(matches(file, site), `${file} is a site path`);
+ }
+ for (const file of ["tests/browser/site.spec.mjs", "playwright.config.mjs", ".github/workflows/browser-tests.yml", ".github/actions/playwright-chromium/action.yml", ".github/site-paths.txt"]) {
+ assert.ok(matches(file, [...site, ...tests]), `${file} runs the browser suites`);
+ }
+ assert.ok(!matches(".github/workflows/build.yml", [...site, ...tests]), "a workflow-only change is not a site change");
+});
diff --git a/scripts/preview-workflows.test.mjs b/scripts/preview-workflows.test.mjs
index 0537efc..920f6aa 100644
--- a/scripts/preview-workflows.test.mjs
+++ b/scripts/preview-workflows.test.mjs
@@ -70,3 +70,27 @@ test("preview-signal.yml has no permissions, no checkout and no expressions", as
assert.doesNotMatch(text, /uses:/, "the signal runs no action");
assert.doesNotMatch(text, /\$\{\{/, "the signal reads nothing from the event");
});
+
+test("previews are offered only for site changes, on both sides", async () => {
+ // build.yml uploads a preview only when `changes` found a site change, and
+ // the publisher explains previews only where one was uploaded. Either half
+ // alone is wrong: the gate without the howto mode tells PRs with nothing to
+ // preview how to preview it, and the mode without the gate changes nothing.
+ const build = await workflow("build.yml");
+ const publish = await workflow("preview-publish.yml");
+ const upload = /- name: Upload the preview\n\s+if: (.*)\n\s+uses: apache\/infrastructure-actions\/website-comment\/preview-artifact@/.exec(build)?.[1];
+ assert.equal(upload, "github.event_name == 'pull_request' && needs.changes.outputs.site == 'true'");
+ assert.match(build, /^ {2}build:\n(?: *\n| {4}.*\n)*? {4}needs: \[changes, lint, browser\]$/m, "build waits for the decision");
+ assert.match(publish, /^\s+howto: with-preview-artifact$/m);
+});
+
+test("the browser suites run on what `changes` decided, and on every other event", async () => {
+ const build = await workflow("build.yml");
+ const job = /^ {2}browser:\n((?: *\n| {4}.*\n)+)/m.exec(build)?.[1] ?? "";
+ assert.match(job, /^ {4}needs: changes$/m);
+ assert.match(
+ job,
+ /^ {4}if: \$\{\{ !cancelled\(\) && github\.event_name != 'push' && \(github\.event_name != 'pull_request' \|\| needs\.changes\.outputs\.browser == 'true'\) \}\}$/m,
+ "skipped only on a pull request that changes neither the site nor the suites",
+ );
+});