Uh oh!
There was an error while loading. Please reload this page.
ci: skip the expensive jobs on documentation-only changes - #311
Open
harshtandiya wants to merge 1 commit into
Open
ci: skip the expensive jobs on documentation-only changes#311harshtandiya wants to merge 1 commit into
harshtandiya wants to merge 1 commit into
Conversation
`ci.yml` and `ui-tests.yml` carried `paths-ignore` for `**.md` and `docs/**`. A workflow skipped by path filtering never reports its checks at all: they sit in Pending forever. `Server` and `Playwright E2E Tests` are required checks on `develop`, so a documentation-only PR was not merely cheap to run, it was unmergeable. A job skipped by a job-level `if:` reports Success instead, and satisfies a required check. So the filter moves from the workflow trigger to the jobs: a `changes` job on `ubuntu-slim` computes one boolean, and every expensive job takes `needs: changes` plus an `if:` on it. The detection lives in a composite action so the four workflows share it. It lists the PR's files via `gh api` rather than `git diff`, which keeps the shallow clone shallow. Only `pull_request` has a base to compare against, so every other event -- push, merge_group, workflow_dispatch, schedule -- falls through to running everything; those guard a branch directly and must never skip. The API call fails open for the same reason: a skipped required check reads as success, so a transient failure must not look like "docs only". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ci.ymlandui-tests.ymlcarriedpaths-ignorefor**.mdanddocs/**. Thatwas worse than it looks. A workflow skipped by path filtering never reports its
checks — they stay Pending.
ServerandPlaywright E2E Testsare requiredchecks in the
protect-developruleset, so a docs-only PR wasn't just skippingCI, it could never be merged. A job skipped by a job-level
if:reports Successand satisfies a required check. Hence the shape of this change: the filter moves
off the trigger and onto the jobs.
A
changesjob onubuntu-slimcomputes one boolean; the expensive job takesneeds: changesand anif:on it. The detection is a composite action in.github/actions/so four workflows share one copy. It reads the PR's file listwith
gh apiinstead ofgit diff, which avoids deepening the clone. Docs pathsare
**.md,docs/**,.claude/**,specs/**.Only
pull_requesthas a base to diff against, so every other event fallsthrough to running everything. The
gh apicall also fails open — a skippedrequired check reads as Success, so a transient API failure must not look like
"docs only".
Per event, for all four gated workflows:
code=falsecode=truecode=truemerge_groupcode=truepushtodevelop/maincode=trueworkflow_dispatch(ui-tests)code=trueDeliberately unchanged:
linter.ymlis not gated.Frappe Linteris the cheapest of theexpensive jobs, and it's the only one that validates the non-
.mddoc paths —check-json,check-yamlandcheck-astin.pre-commit-config.yamlareunscoped, so gating it would stop catching a malformed
.claude/settings.jsonon exactly the PRs that touch it. Its
scheduletrigger,workflow_dispatch,the
if: github.event_name != 'schedule'condition and the separateVulnerable Dependency Checkjob are all untouched.name:was renamed — the five required contexts are matchedas strings.
concurrencygroups and existing permissions are as they were. Thechangesjobs declare their own
contents: read/pull-requests: read.pr-tittle-check.ymlandbackport.ymlare cheap or event-specific.Verified:
actionlintreports nothing new (its one SC2086 hit is pre-existing inthe untouched
linter.ymland identical ondevelop). Therun:script wasextracted from
action.ymland executed against a stubbedghfor all ninecases — docs-only, mixed, code-only,
.github/**, the four non-PR events, and afailing
gh— each giving the expectedcode=. On this PR the gate correctlyread
.github/**as code and every job ran.Not verified: a real docs-only PR end to end, and the
merge_grouppath, neitherof which can be exercised before this merges. Both are covered by the harness
only.