Automate SDK bump PRs, and fix the CODEOWNERS and CDN updater bugs - #101
Conversation
The CODEOWNERS file referenced @PSPDFKit/nickel, a team that does not exist, so no review was ever auto-requested. Point it at @PSPDFKit/web, which has write access to this repository. The CDN updater keyed its map on "gatsby" while the directory is "gatsbyjs", and omitted nuxtjs and the Salesforce README entirely. Those three files were left pinned at 1.8.0, 1.3.0 and 1.0.0 while every other example moved to 1.18.0. Correct the key, add the missing entries, and validate all paths before writing so a moved file fails the run instead of silently half-updating an example. Both scripts now take the target version as a required argument instead of resolving @latest at install time, so a release landing mid-run cannot produce a bump whose title and lockfiles disagree.
Adds a daily workflow that checks the npm latest dist-tag, bumps every example, and opens a pull request. @PSPDFKit/web is requested for review through CODEOWNERS rather than by the workflow, so no token needs permission to resolve org teams. Detection refuses to act twice on the same release: it exits when the repository is already on the version, when the branch exists, or when a pull request for it was opened before, so a bump closed without merging is not reopened on the next run. Prereleases are rejected outright, so nightly builds never trigger a bump. Biome and the e2e suite run inside the job before the pull request is opened. A pull request created with GITHUB_TOKEN does not trigger the Biome or Playwright workflows, and main requires no status checks, so a bump would otherwise arrive with nothing having verified it. A failing suite still opens the pull request, as a draft, with the report attached. pnpm is pinned to 10 because pnpm 11 stops reading the pnpm.overrides field in package.json, which would silently drop the security overrides added in #98.
This comment has been minimized.
This comment has been minimized.
Refuse to reopen a closed bump, fail loudly when a run stops part-way, and reject a CDN map key or file that matches nothing. Exempt the salesforce README, whose version is an illustration rather than a pin.
miguelcalderon
left a comment
miguelcalderon
left a comment
There was a problem hiding this comment.
The PR looks solid to me. The verification table in the description made this easy to review.
I've left some comments, none should be a blocker, just for your consideration. Please make sure to create an issue for generalizing the hardcoded example list in update-nutrient-in-examples.sh, I agree it belongs in a follow-up.
Thank you for this! This is removing overhead.
| stale="$(grep -rEn "pspdfkit-web@[0-9]+\.[0-9]+\.[0-9]+" examples/ \ | ||
| --exclude-dir=node_modules --exclude-dir=dist \ | ||
| --exclude-dir=.next --exclude-dir=.nuxt \ | ||
| | grep -v "pspdfkit-web@${VERSION}" \ | ||
| | grep -v '^examples/salesforce/README.md:' || true)" |
There was a problem hiding this comment.
Two small blind spots in this filter:
grep -v "pspdfkit-web@${VERSION}"drops the whole line, so a line containing both an updated and a stale reference (e.g. a README listing two CDN URLs on one line) would hide the stale one.- The dots in
${VERSION}are regex metacharacters, so the exclusion is slightly looser than intended.
Both are covered by making the first grep emit one match per line and the exclusion a fixed string:
stale="$(grep -rEon "pspdfkit-web@[0-9]+\.[0-9]+\.[0-9]+" examples/ \
--exclude-dir=node_modules --exclude-dir=dist \
--exclude-dir=.next --exclude-dir=.nuxt \
| grep -vF "pspdfkit-web@${VERSION}" \
| grep -v '^examples/salesforce/README.md:' || true)"Low probability in practice, so fine as a follow-up if you prefer.
There was a problem hiding this comment.
Fixed in #103, using your form verbatim.
Reproduced the whole-line case before changing it, and it is worse than "hides the stale one" — the output is completely empty:
$ echo "pspdfkit-web@1.21.0 and pspdfkit-web@1.8.0 on one line" > /tmp/t.txt
$ grep -rEn "pspdfkit-web@[0-9]+\.[0-9]+\.[0-9]+" /tmp/t.txt | grep -v "pspdfkit-web@1.21.0"
$ grep -rEon "pspdfkit-web@[0-9]+\.[0-9]+\.[0-9]+" /tmp/t.txt | grep -vF "pspdfkit-web@1.21.0"
/tmp/t.txt:1:pspdfkit-web@1.8.0
examples/salesforce/force-app/.../Nutrient_InitNutrient.page already carries two CDN references, on lines 7 and 16, so the shape is one edit away from being real here.
| if [ -n "${requested}" ]; then | ||
| latest="${requested}" | ||
| else | ||
| latest="$(curl -fsSL --retry 3 --retry-delay 2 "${DIST_TAGS_URL}" | jq -er '.latest')" | ||
| fi |
There was a problem hiding this comment.
A manually requested version is taken at face value: nothing checks it exists on the registry, and nothing prevents it being older than current (the script only tests inequality, so a dispatch with 1.17.0 would happily open a downgrade PR titled as a bump).
The nonexistent-version case fails soon enough (the first npm install errors), just with a less obvious message. The downgrade case may even be a feature (rollback) — if so, a one-line comment saying it's intentional would help; if not, a dist-tags/npm view existence check here would make both fail in the check step with a clear message.
There was a problem hiding this comment.
Both, in the end — #103 adds a registry existence check and keeps the downgrade, with a comment saying it is deliberate.
The typo case had no upside, and it now fails in the check step with the version named rather than as an npm 404 well into the bump, after the branch already exists:
$ ./scripts/check-nutrient-update.sh 1.17.99
@nutrient-sdk/viewer@1.17.99 is not published on the registry.
The downgrade is a feature: dispatching an older version is the only rollback path there is, so it stays, now documented as such. The check only runs for a dispatched version, so the scheduled path makes no extra request.
| - name: Fail the run when the e2e suite failed | ||
| if: steps.check.outputs.should_update == 'true' && steps.e2e.outcome != 'success' | ||
| env: | ||
| VERSION: ${{ steps.check.outputs.version }} | ||
| run: | | ||
| echo "::error::E2E failed for ${VERSION}. The pull request was opened as a draft," | ||
| echo "::error::and GitHub does not request code owners on drafts." | ||
| exit 1 |
There was a problem hiding this comment.
Once this draft exists, check-nutrient-update.sh will skip that version on every future run ("a pull request already exists") — which is the right behavior for a deliberately declined bump, but it also means a transient e2e flake permanently parks the version on a human, and re-running the e2es isn't possible from the PR itself (no checks are attached to it).
The recovery path (mark ready + verify by hand, or close the PR, delete the branch, and workflow_dispatch again) isn't written down anywhere. Suggest adding it to the draft PR body in the failure branch above, so the person looking at the draft knows what to do without archaeology.
There was a problem hiding this comment.
Added to the draft body in #103. The failure branch now appends a "This draft needs a human" section spelling out the three ways out — accept it after checking the run log, close and delete the branch then re-dispatch, or close it and leave the branch so the version is never proposed again.
Rendered both variants by running the step's own script to confirm the section appears only on failure.
3 participants
⚡ TL;DR
A new
@nutrient-sdk/viewerrelease now opens its own bump PR, already formatted and e2e-tested, instead of waiting for someone to notice. Two silent bugs that let stale versions sit in the repo for months are fixed along the way.🎯 What this improves
@PSPDFKit/nickel, a team that does not exist, so GitHub silently requested nobody on every PR in this repo.examples/gatsbyjssat eight minor versions behind because of a one-character typo nothing was checking.🔧 What changed
npm run update-nutrient-version; the CDN updater keyed one example asgatsbywhile the directory isgatsbyjs, so that entry was never looked up and never updated; CODEOWNERS named a nonexistent team..github/workflows/update-nutrient-sdk.ymlruns daily (and onworkflow_dispatch, optionally against a given version).scripts/check-nutrient-update.shdecides whether to act and refuses to start a bump already in flight.update-nutrient-in-cdn.jsrejects a map key matching no directory, rejects a mapped file that is missing, and rejects a replace that matched nothing. A workflow-level grep then catches any example carrying a CDN tag that the map does not know about at all.🔍 Root cause
cdnOcurrencesis a hand-maintained map from example name to file list. A key that matches no directory is simply never looked up, so thegatsby/gatsbyjstypo produced no error, no warning, and no diff.examples/nuxtjshad a second variant of the same failure: it was absent from the map entirely./pspdfkit-web@([0-9]+.[0-9]+.[0-9]+)?/gmade the version group optional, so a file with no version still "matched", got rewritten byte-identical, and was reported as updated.@PSPDFKit/nickelreviewed nothing and said nothing.🤔 Why
examples/for CDN tags, removing the hand-maintained list entirely. Rejected for this PR: it trades a small include-list for an exclude-list (examples/salesforce/README.mddocuments the CDN URL with a worked@1.0.0example and must never be bumped), and it rewrites a script this PR is already changing. Worth doing separately.GITHUB_TOKENdoes not trigger the Biome or Playwright workflows, andmainrequires no status checks, so the bump would otherwise arrive with no signal at all.🧪 How to test
gatsbyjskey togatsbyand it now refuses to run:node scripts/update-nutrient-in-cdn.js webpack 9.9.9 # Keys matching no example directory: gatsbynpx @biomejs/biome@1.9.4 ci . ./scripts/check-biome-version.shgh pr createstep fails with a permissions error.nuxtjsis on 1.3.0 andgatsbyjson 1.8.0. This PR does not bump them; it makes the next scheduled run bump them and fail loudly if it cannot.examples/only:pnpm run formatwrites unsafe Biome fixes repository-wide, so the commit step usesgit add -u examples/and discards the rest.update-nutrient-in-examples.shstill hard-codes its 20 example directories. A new example is silently never bumped, which is the same class of drift this PR fixes in the CDN map. Left out because generalizing it changes which examples get bumped (wasm-benchmarkpins a range deliberately) and needs its own verification.