Repository navigation
fix(scaffolds): call wp-shared-workflows at @v1 and let .nvmrc pick Node - #89
aryanjasala wants to merge 5 commits into
Conversation
The lint-css, lint-js and lint-php scaffolds defaulted wsw_ref to the deleted v1.0.0/task/ci-lint-workflows branch, so a workflow scaffolded with the defaults failed at startup. test-measure defaulted to the stale release/v1.0.0 branch and cd-wporg hard-coded @v1.0.0. All now call @v1, and the lint templates drop the outdated EXPERIMENTAL header. node_version defaults to empty in the six scaffolds that had 22, and the templates leave node-version out when it is empty, so the shared workflow reads the project's .nvmrc (wp-shared-workflows v1.1.0). An explicit --node_version still pins a version. test-measure's default WordPress versions gain 7.1.
There was a problem hiding this comment.
🟡 Changes recommended
The six updated CI templates lack regression coverage for optional node-version rendering, leaving the empty-with: failure unprotected.
1 open issue
What changed in this PR
Updates the CI scaffolds to use shared-workflow @v1 references and let project .nvmrc files determine Node.js versions by default. These changes affect newly generated workflows, not existing project files.
Changes:
- Replace stale workflow references and remove obsolete experimental comments.
- Make Node.js overrides optional across six scaffolds, omitting unused YAML inputs.
- Add WordPress 7.1 to the test-measure matrix and document the fixes.
Paths below are relative to node-packages/wp-tooling/.
| File | Description |
|---|---|
scaffolds/ci/test-php/templates/ci-test-php.yml.mustache |
Conditionally emits the Node.js override. |
scaffolds/ci/test-php/scaffold.json |
Defaults Node.js selection to .nvmrc. |
scaffolds/ci/test-measure/scaffold.json |
Defaults to v1 and adds WordPress 7.1. |
scaffolds/ci/test-js/templates/ci-test-js.yml.mustache |
Makes the Node.js input conditional. |
scaffolds/ci/test-js/scaffold.json |
Removes the default Node.js pin. |
scaffolds/ci/test-a11y/templates/ci-test-a11y.yml.mustache |
Omits with: when no override is supplied. |
scaffolds/ci/test-a11y/scaffold.json |
Defaults Node.js selection to .nvmrc. |
scaffolds/ci/lint-php/templates/ci-lint-php.yml.mustache |
Removes obsolete experimental comments. |
scaffolds/ci/lint-php/scaffold.json |
Replaces the deleted branch default with v1. |
scaffolds/ci/lint-js/templates/ci-lint-js.yml.mustache |
Removes stale comments and makes Node.js optional. |
scaffolds/ci/lint-js/scaffold.json |
Uses v1 and removes the Node.js pin. |
scaffolds/ci/lint-css/templates/ci-lint-css.yml.mustache |
Removes stale comments and conditionally emits with:. |
scaffolds/ci/lint-css/scaffold.json |
Uses v1 and defaults to .nvmrc. |
scaffolds/ci/cd-wporg/templates/cd-wporg.yml.mustache |
Calls the deployment workflow at @v1. |
scaffolds/ci/build/templates/ci-build.yml.mustache |
Conditionally emits the Node.js override. |
scaffolds/ci/build/scaffold.json |
Removes the default Node.js pin. |
CHANGELOG.md |
Records reference, Node.js, and matrix updates. |
Review effort: Balanced
…affold ci/cd-wporg passed inputs and secrets cd-wp-org.yml never had (plugin-slug, svn-username, svn-password) and deployed an artifact that nothing built, so it failed before any job ran. It now builds with ci-build.yml, packages with wp-tooling release:zip (which honours .distignore), unpacks with python3 (self-hosted runners have no unzip) and deploys with cd-wp-org.yml's slug, tag, artifact-name and WP_ORG_USERNAME/WP_ORG_PASSWORD. New inputs: wsw_ref, build_command, assets_path. The descriptions wp-tooling list prints drop the EXPERIMENTAL wording and the deleted-branch advice. A new test renders every ci/* scaffold, with and without node_version, and checks each reusable-workflow call against a snapshot of the wp-shared-workflows v1 workflow_call contracts: no empty with: or secrets: block, every call at @v1, no unknown input or secret, and every required one present. It fails on the old cd-wporg template.
There was a problem hiding this comment.
🟡 Changes recommended
The artifact upload drops packaged hidden files, potentially removing access protections, while unescaped apostrophes in custom build commands can produce invalid workflow YAML.
2 open issues
1 resolved since last review
Review effort: Balanced
| build-command: '{{build_command}}' | ||
| upload-artifact: true | ||
| artifact-name: wporg-package | ||
| artifact-path: dist/plugin |
| uses: rtCamp/wp-shared-workflows/.github/workflows/ci-build.yml@{{wsw_ref}} | ||
| with: | ||
| install-composer-deps: true | ||
| build-command: '{{build_command}}' |
There was a problem hiding this comment.
with --build_command="npm run build && cp 'readme.txt' dist/plugin/" the rendered cd-wporg.yml doesn't parse (actionlint: could not parse as YAML: did not find expected key). The default command renders fine because it has no '.
A block scalar avoids escaping altogether, since YAML takes the line as written, so ', " and # are all safe. I checked that the default and a command containing all three render valid YAML with the exact command.
| build-command: '{{build_command}}' | |
| build-command: >- | |
| {{build_command}} |
npx falls back to the registry when the project has no local install, and nobody owns the unscoped wp-tooling name there. Also drop the v1 contract test and its fixture: it compared the templates with a hand-copied snapshot, so it could not catch upstream changes, and wp-shared-workflows already checks its own callers in CI.
…-refs # Conflicts: # node-packages/wp-tooling/CHANGELOG.md
| uses: rtCamp/wp-shared-workflows/.github/workflows/ci-build.yml@{{wsw_ref}} | ||
| with: | ||
| install-composer-deps: true | ||
| build-command: '{{build_command}}' |
There was a problem hiding this comment.
with --build_command="npm run build && cp 'readme.txt' dist/plugin/" the rendered cd-wporg.yml doesn't parse (actionlint: could not parse as YAML: did not find expected key). The default command renders fine because it has no '.
A block scalar avoids escaping altogether, since YAML takes the line as written, so ', " and # are all safe. I checked that the default and a command containing all three render valid YAML with the exact command.
| build-command: '{{build_command}}' | |
| build-command: >- | |
| {{build_command}} |
…-refs # Conflicts: # node-packages/wp-tooling/CHANGELOG.md
What this PR does
Fixes the CI scaffolds so a workflow generated with the defaults runs.
ci/lint-css,ci/lint-jsandci/lint-phpdefaultedwsw_reftov1.0.0/task/ci-lint-workflows, a rtCamp/wp-shared-workflows branch that no longer exists, so the generated workflow failed before any job started. They now call@v1, like the other CI scaffolds.Closes
N/A
Changes
ci/lint-css,ci/lint-js,ci/lint-php:wsw_refdefaults tov1, and the templates drop the outdated "EXPERIMENTAL ... switch to@v1once the PR is merged" header.ci/test-measure:wsw_refdefaults tov1instead of the stalerelease/v1.0.0branch, and the default WordPress versions gain 7.1.ci/cd-wporg: callscd-wp-org.yml@v1instead of the hard-coded@v1.0.0.node_versioninci/build,ci/lint-css,ci/lint-js,ci/test-a11y,ci/test-jsandci/test-phpdefaults to empty. The templates leavenode-versionout when it is empty, so the shared workflow uses the project's.nvmrc(rtCamp/wp-shared-workflows v1.1.0 reads it). The old default of22overrode.nvmrc;--node_versionstill pins a version.### Fixed.How I verified
ci/*template with the engine's ownrender(), once withnode_versionempty and once with24: all 16 workflows passactionlint. The first attempt caught two templates (ci/lint-css,ci/test-a11y) wherenode-versionwas the onlywith:key, which would have rendered an emptywith:; the wholewith:block now sits inside the section there.ci/test-measure, because the renderer keeps lines that hold only a tag.npx jest --ci: 82 suites, 1378 tests pass.npm run lintpasses.Reviewer notes
@v1by hand.