ci(build): replace Danger with a dependency-free PR title check - #93
Merged
Conversation
Danger JS read the pull request title and reported on it through the questdb-butler account, using a personal access token held in the DANGER_GITHUB_TOKEN secret. Nothing about the job needed a separate identity, and the token's expiry would have quietly stopped the check: Danger is not a required status here, so it would have failed open rather than loudly. ci/validate-pr-title/check.js now does the same two things on the workflow's own GITHUB_TOKEN: it posts the commit status and leaves a comment explaining a rejection, updating that comment in place while the title stays wrong and deleting it once the title is fixed. The yarn global add danger step and dangerfile.js are gone, and the scripts use only the Node standard library. DANGER_GITHUB_TOKEN can be deleted from the repository secrets once this has settled. The status context is "PR title", not the "Danger" that Danger posted. The rename is safe here and was checked rather than assumed: branch protection on main requires only the four questdb.java-questdb-client Azure contexts, and every open pull request that carries a Danger status carries a green one, so nothing is left waiting on a check that stopped reporting. The copy in questdb/questdb cannot do this, because its master ruleset names "Danger" exactly. The subType list is unchanged. It already matches what this repository merges: across 69 merged and 8 open pull requests only qwp, build, ilp, core and conf appear, all of them already allowed. The rules gain one thing, the Conventional Commits breaking-change marker, so feat(qwp)!: passes. That is strictly more permissive and cannot redden a title that passes today. validate.test.js loses a long-standing hole: testInvalid passed a bare undefined onError, so the assertion was satisfied by a ReferenceError rather than by the title being rejected, and every negative case passed against a validator that accepts everything. Replacing the regex body with an unconditional accept now fails the suite; before this change it passed. check.test.js covers the reporting against a stubbed API, including the comment lifecycle, the merge-group path, and the fail-closed behaviour that publishes a verdict even when the run comes apart. Two details are load-bearing and pinned by tests: the status must be posted on pull_request.head.sha, since GITHUB_SHA on a pull_request event is the throwaway merge commit and a status there is invisible to the pull request; and synchronize has to stay in the trigger list, because the status is attached to a commit and a new head needs its own verdict. The tests run in their own job rather than as a first step of the job that posts the verdict, so a failing test cannot stop check.js from running and leave a corrected title still carrying its rejection comment.
readme.md and the comment above CONTEXT both said questdb/questdb could not rename its status context away from "Danger". It has: questdb/questdb#7599 moves it to "PR title", the same string this repository posts, paired with a hand edit of the master ruleset. What is actually different survives the rename and is the part worth keeping, because it is about what a rename costs rather than what the context is called. Here the string is free: branch protection on main requires only the four questdb.java-questdb-client Azure contexts, so nothing ever waited on "Danger" and no open pull request was left behind a check that stopped reporting. There the ruleset names the context, so the code and the ruleset are a contract, and the gap between changing one and the other is a gap in which nothing merges.
The workflow name and the status context are both "PR title", so a pull request showed three rows that read the same way: the "PR title" status, plus the "PR title / test" and "PR title / validate" check runs GitHub names after the workflow and its jobs. The rules' tests move to pr_title_rules.yml behind a path filter, so they run when ci/validate-pr-title or either workflow changes and stay off every other pull request. The reason they are not a step of the job that posts the status is unchanged and now structural rather than conventional: run there, a failing or flaky test stops check.js from running at all, no status is posted, and the pull request sits behind a check that is merely missing. Separate workflows cannot do that to each other. A pull request now shows the status and one "PR title / validate" row, and a third only when the checker itself is under change, which is when the tests are worth looking at.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Third and last of the PR-title-check ports, after questdb/questdb#7599 and questdb/questdb-enterprise#1202. Unlike the enterprise one this is a genuine replacement: Danger really does run here.
Danger read the title and reported through the questdb-butler account using a PAT in the
DANGER_GITHUB_TOKENsecret (last updated 2026-01-28). Nothing about the job needed a separate identity, and becauseDangeris not a required status in this repo, that token expiring would have failed the check open rather than loudly.check.jsnow does the same work on the workflow's ownGITHUB_TOKEN, with noyarn global add dangerand no dependency at run time.DANGER_GITHUB_TOKENcan be deleted from repository secrets once this settles.The
Danger→PR titlerename is safe here — checked, not assumedmainrequiresquestdb.java-questdb-clientAzure contexts — notDangerDangerstatusSo no ruleset or branch-protection edit is needed, and no open PR ends up waiting on a check that stopped reporting. questdb/questdb has since moved to the same
PR titlestring in questdb/questdb#7599, but had to pay for it: itsmasterruleset names the context, so the rename had to be paired with a hand edit of the ruleset, and the gap between the two is a gap in which nothing can merge. Here the string was free.The rules are essentially unchanged
The subType list already matches what this repo merges. Across 69 merged + 8 open PRs, only
qwp(33),build(15),ilp(11),core(5) andconf(1) appear — every one already allowed. The current validator rejects 2/69 merged, and both areBump version to 1.3.x-SNAPSHOTPRs opened byapp/github-actions.One rule is added: the Conventional Commits breaking-change marker, so
feat(qwp)!:passes. Strictly more permissive — it cannot redden anything that passes today.validate.test.jswas vacuoustestInvalidpassed a bare undefinedonError, soassert.throwswas satisfied by aReferenceErrorrather than by the title being rejected — every negative case passed against a validator that accepts everything. Demonstrated rather than asserted:validate.test.jsvalidate.test.jsCarried over from the enterprise port
findCommentreturns one match, so a PR carrying two sheds one per run and keeps the rest.check.jsentirely, leaving a corrected title still carrying its rejection comment. On the enterprise PR this was not hypothetical — a checkout failure took out the test job whilevalidatestill posted its verdict.Known wart, unchanged by this PR
The automated
Bump version to 1.3.x-SNAPSHOTrelease PRs do not match the format and go red. Danger rejects them today too and they merge anyway, since the check is not required — so this is not a regression. If the check is ever made required, retitle thembuild: bump version to 1.3.x-SNAPSHOT; barebuild:is already accepted.Verification
node validate.test.jsandnode check.test.jspass;check.test.jscovers 10 reporting scenarios.