ci(playwright): restore fork PR support on knowledge-graph workflow - #30561
ci(playwright): restore fork PR support on knowledge-graph workflow#30561chirag-madlani wants to merge 2 commits into
Conversation
Mirrors the pattern #30485 established on playwright-postgresql-e2e.yml to the RDF/knowledge-graph workflow, which #30310 also regressed to pull_request-only. Fork PRs against RDF/ontology paths were previously short-circuited by the `!fork` check on the second job — they built the distribution but silently skipped the entire Playwright run, so RDF work from external contributors ran zero E2E coverage. Changes: - Add pull_request_target trigger with the same paths filter used by pull_request so the scope stays RDF-focused. - Partition the concurrency group by github.event_name so pull_request and pull_request_target runs on the same PR don't cancel each other. - Introduce a `gate` job that waits for Team Label reconciliation on fork synchronize events, reads the reconciled live PR labels via the GitHub API, and computes a should_run decision: workflow_dispatch → run draft → skip labeled event, label != "safe to test" → skip same-repo PR + pull_request → run fork PR + pull_request_target + reconciled "safe to test" label → run everything else → skip - Wire `build` and `playwright-rdf-postgresql` through `needs: gate` with a `needs.gate.outputs.should_run == 'true'` guard. Removes the now-redundant `!github.event.pull_request.head.repo.fork` short- circuit from the RDF job. - Fix the two `actions/checkout@v7` steps to explicitly check out `github.event.pull_request.head.sha` under pull_request_target (base.sha is wrong for testing fork code) and set `allow-unsafe-pr-checkout: true` — the gate already restricted this event to safe-to-test-labelled forks, and the build/test steps consume the fork tree inside docker containers. The workflow doesn't have a playwright-summary job or a named required-check consumer, so no name-collision rename (#30476) is needed here. `pull_request` vs `pull_request_target` runs on the same PR each report their own separate check-run status; gate-skipped runs finish trivially with `should_run=false` and don't perform any docker work. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| uses: lewagon/wait-on-check-action@9312864dfbc9fd208e9c0417843430751c042800 # v1.7.0 | ||
| with: | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
| check-name: Team Label | ||
| repo-token: ${{ secrets.GITHUB_TOKEN }} | ||
| wait-interval: 10 |
There was a problem hiding this comment.
The wait action uses its 60-second check-discovery timeout for every fork event. When the Team Label workflow remains queued beyond that window, this gate fails instead of skipping or running the approved fork, adding a noisy failure and preventing RDF E2E execution for that event.
✅ Playwright Results — workflow succeededValidated commit ✅ 536 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 48m 59s ⏱️ Max setup 2m 53s · max shard execution 14m 30s · max shard-job elapsed before upload 18m 6s · reporting 6s 🌐 199.69 requests/attempt · 2.86 app boots/UI scenario · 16.32% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Per @chirag Madlani review — two follow-up gaps confirmed: 1. This workflow is required in the merge queue. 2. `Playwright RDF (Knowledge Graph + Ontology)` is a required branch-protection check. Add merge_group support: - Add `merge_group:` to on: triggers. - Add merge_group to the gate's always-run case: workflow_dispatch | schedule | merge_group. Add name-collision handling (same pattern #30476 established for playwright-postgresql-e2e.yml): - Rename the existing execution job's display name from `Playwright RDF (Knowledge Graph + Ontology)` to `RDF Playwright execution` — internal identity only, no longer the required check name. - Add a `playwright-rdf-summary` job with `if: always() && !cancelled()` that publishes a dynamic check name: gate succeeds, should_run=true → `Playwright RDF (Knowledge Graph + Ontology)` gate fails (crash/cancelled) → `Playwright RDF (Knowledge Graph + Ontology)` + hard fail labeled event, non-safe-to-test → `... (label ignored)` gate succeeds, should_run=false → `... (skipped)` - Three steps inside the summary job: "Guard against missing gate decision" — fails loudly when gate itself did not produce a valid should_run "Report gate-skipped run as green" — no-op success for legitimate skips (redundant sibling event, fork without safe-to-test) "Fail on upstream failure" — propagates build / execution job failures under the required check name so the PR fails visibly Without this, both events on a same-repo PR would publish check-runs under `Playwright RDF (Knowledge Graph + Ontology)`; the pull_request_target sibling finishes in ~15s with a green skip while the real pull_request run is still compiling, making the PR appear mergeable before any RDF test executes — the same class of hole #30476 fixed for playwright-summary on postgresql-e2e. No branch-protection update needed — the required check name is preserved end-to-end; only its publisher moved from the underlying execution job to the always-running facade. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed both follow-ups in c2c24f266a. 1.
2. Name-collision handling for the required check
Three guard steps inside the facade:
No branch-protection update needed — the required check name is preserved end-to-end; only its publisher moved from the underlying execution job to the always-running facade. Behaviour matrix:
Diff on this commit: +87 / −8 lines (job rename + facade job + updated trigger + gate case). |
|
The Two open items remain from the original inline review: P1 — cancel-in-progress (thread marked resolved, fix not applied) The concurrency block still has The earlier suggestion still applies: P2 — Team Label check discovery timeout (thread still open) The |
Code Review ✅ ApprovedRestores fork PR support on the knowledge-graph Playwright workflow by adding pull_request_target triggers, a concurrency group partition, and a gating job. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source |
Summary
Mirrors the pattern #30485 established on `playwright-postgresql-e2e.yml` to the RDF/knowledge-graph workflow, which #30310 also regressed to `pull_request`-only. Fork PRs against RDF/ontology paths were previously short-circuited by a `!github.event.pull_request.head.repo.fork` check on the RDF job — they built the distribution but silently skipped the entire Playwright run, so external RDF contributions had zero E2E coverage.
Changes
Not applicable here
This workflow has no `playwright-summary` job or named required-check consumer, so the check-name-collision rename (#30476) isn't needed. `pull_request` vs `pull_request_target` runs on the same PR each report their own separate check-run status; gate-skipped runs finish trivially with `should_run=false` and don't perform any docker work.
Behaviour matrix
Not in scope
Diff scope
```
.../playwright-knowledge-graph-postgresql-e2e.yml | 158 ++++++++++++++++++++-
1 file changed, 152 insertions(+), 6 deletions(-)
```
Sizeable but structurally minimal for the scope: trigger + concurrency + gate + checkout ref fixes.
Test plan
🤖 Generated with Claude Code
Greptile Summary
This revision expands and hardens RDF Playwright workflow orchestration.
Confidence Score: 3/5
The PR is not yet safe to merge because non-safe label events can still cancel active fork coverage and delayed Team Label checks can block approved runs.
The event-name concurrency partition does not distinguish pull_request_target label events for the same PR, so a gate-skipped label run can cancel the authoritative run; the unchanged 60-second check-discovery window also leaves approved fork runs vulnerable to queue delays.
Files Needing Attention: .github/workflows/playwright-knowledge-graph-postgresql-e2e.yml
Important Files Changed
Reviews (2): Last reviewed commit: "add merge_group trigger + name-collision..." | Re-trigger Greptile