-
Notifications
You must be signed in to change notification settings - Fork 0
ci(gha): install pnpm before use; align release to appsyncjs (Node 22/24, NPM_TOKEN) #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…de 22/24, registry auth, caching)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Release job lacks an event guard and can run on pull_request events in pre mode, risking unintended publishes.
- pnpm is installed with
version: latest, reducing reproducibility and cache stability; pin the major/exact version to matchpackageManager. - Consider
always-auth: trueforactions/setup-nodewhen configuring npm auth to harden publishing. git push --follow-tags || truehides push failures, making release issues harder to detect and recover from.
Summary of changes
- Renamed workflow to “CI — test → gate → release”.
- Expanded triggers to all branches/tags on push and selected pull_request events; added concurrency (cancel in progress) and default read-only permissions.
- Test job: switched to
matrix.node-version(22.x, 24.x), installs pnpm viapnpm/action-setup@v4, sets up Node with pnpm cache andcache-dependency-path, installs deps, runs format and typecheck. - New gating:
determine_releasedepends on tests, installs pnpm/Node 24.x, computesis_prereleaseandis_releasefrom Changesets status and branch name. - Release job: explicit git author config, installs pnpm, sets up Node with npm registry auth using
NPM_TOKEN, installs, builds, runs Changesets status/version/publish, and pushes tags. - General alignment with appsyncjs patterns and pnpm caching improvements.
.github/workflows/release.yml
Outdated
| if: needs.determine_release.outputs.is_release == 'true' || needs.determine_release.outputs.is_prerelease == 'true' | ||
| permissions: | ||
| contents: write | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The release job can run on pull_request events whenever Changesets is in pre mode (is_prerelease == 'true'). Because secrets.NPM_TOKEN is available to PRs from the same repository, this can unintentionally publish from a PR context. Restrict publishing to push events to prevent accidental releases from PRs.
Suggestion
Tighten the job condition to only allow releases on push events:
if: >-
${{ github.event_name == 'push' && (needs.determine_release.outputs.is_release == 'true' || needs.determine_release.outputs.is_prerelease == 'true') }}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| # Use the pnpm version pinned in package.json `packageManager` to avoid conflicts | ||
| version: latest | ||
| run_install: false | ||
| - run: pnpm install --frozen-lockfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using version: latest for pnpm reduces reproducibility and can cause unexpected breakage when pnpm releases new majors/minors. It also fragments caching across jobs if a different pnpm version is resolved at runtime. Prefer pinning pnpm to the major (or exact) version used in your repo’s packageManager field, and apply consistently across all jobs.
Suggestion
Pin pnpm to your repo’s intended version (update all occurrences):
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 9 # align to your packageManager (e.g., [email protected])
run_install: falseReply with "@CharlieHelps yes please" if you'd like me to add a commit applying this across the workflow.
|
@CharlieHelps Please implement all suggestions and ensure the repo checksare passing before requesting nexts review |
Expand this to see my work.
|
…h; pin pnpm via packageManager and fix Prettier config resolution
…hen changeset status cannot diff from main)
|
Implemented the requested review updates to Changes
Also added a workspace devDependency to resolve local Prettier config during CI formatting:
Verification# Formatting (root)
$ pnpm -w run fmt:check
All matched files use Prettier code style!
# Type check (root)
$ pnpm -w run typecheck:tsgo
tsgo: no-op (no TS sources in this repo)
# Build (root)
$ pnpm -w run build
noop
Notes
Requesting another review from @CharlieHelps and @mikecbrant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the workflow hardening looks solid: push-only release gating, pnpm setup via package_json_file, npm auth with always-auth, and visible tag push failures. Two improvements: ensure pnpm install also has NODE_AUTH_TOKEN set when always-auth is enabled, and log a warning when the Changesets status fallback is used. No issues found in the added devDependency and lockfile updates.
Additional notes (1)
- Maintainability |
.github/workflows/release.yml:81-83
Silencingchangeset statusfailures by writing an empty JSON can hide genuine errors and make gating decisions less transparent in logs. Emitting a warning when the fallback is triggered improves observability without breaking the tolerant behavior for PR merge refs.
Summary of changes
- Renamed workflow to “CI — test → gate → release” and broadened triggers to all branches, tags, and selected PR events.
- Added concurrency (cancel in progress) and default least-privilege permissions; jobs elevate as needed.
- Test job: installs pnpm via
pnpm/action-setup@v4usingpackage_json_file, sets up Node with pnpm cache and lockfile path, runs install, format, and typecheck. - New determine_release job: installs pnpm/Node 24.x, runs install, computes
is_prereleasewith a tolerant Changesets status fallback, and setsis_releasebased on branch. - Release job: now gated to run only on push events and when release/prerelease flags are set; configures git author, installs pnpm via
package_json_file, sets up node withregistry-urlandalways-auth, sets auth token for setup and publish, installs, builds, publishes, and pushes tags without swallowing failures. - Added a workspace devDependency to
packages/eslint-config/package.jsonfor@mikecbrant/prettier-config; lockfile updated accordingly.
Fix the release workflow so pnpm is always available and Changesets can publish with NPM_TOKEN, mirroring the working sequence from mikecbrant/appsyncjs.
Summary
cache-dependency-path: pnpm-lock.yaml.registry-url: https://registry.npmjs.orgNODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}(also set on the publish step).Why this fixes the failures
package_json_fileavoids any conflict with the repo’s pinned pnpm version inpackageManager.Acceptance criteria mapping
Refs APPSYNC-17.