ci: Use Docker container for npm-publish workflow#657
Conversation
📝 WalkthroughWalkthroughGitHub Actions publish workflows were containerized (using a Docker image), removed explicit Node.js/pnpm/Go/Deno setup steps, changed prerelease detection to look for a hyphen in versions, and consolidated dependency install and npm-publish steps with an NPM_TAG decision and public package visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Container as Container (coasys/ad4m-ci-linux)
participant Runner as Job Steps
participant Registry as npm Registry
GH->>Container: start job in container
Container->>Runner: run checkout, read package versions
Runner->>Runner: detect prerelease? (version contains "-")
alt prerelease
Runner->>Runner: set NPM_TAG = next
else stable
Runner->>Runner: set NPM_TAG = latest
end
Runner->>Runner: pnpm install --no-frozen-lockfile
Runner->>Runner: set package visibility to public (core packages)
Runner->>Registry: publish packages with NPM_TAG & public access
Registry-->>Runner: publish results
Runner-->>GH: job completes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lucksus
left a comment
lucksus
left a comment
There was a problem hiding this comment.
Using our docker image, we can remove setup steps in the workflow, no?
The npm-publish job was failing because node-bbs-signatures couldn't find pre-built binaries for Node 18 on the GitHub runner. Use the same Docker image (coasys/ad4m-ci-linux) that CircleCI uses, which has all dependencies pre-installed. Removed redundant setup steps since Docker image already has: - Node.js 18 - pnpm - Go - Deno - Linux dependencies Updated: - .github/workflows/publish.yml - .github/workflows/publish_staging.yml
data-coasys
force-pushed
the
fix/publish-workflow-docker
branch
from
b9c6268 to
6891d18
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish_staging.yml (1)
393-400:⚠️ Potential issue | 🟡 MinorPrerelease detection is incomplete — only matches literal
"prerelease"substring.The check
[[ $STRING == *"prerelease"* ]]fails to catch pre-release versions using other common semantic versioning identifiers. Project history shows versions like0.11.2-dev.4and0.11.2-dev.3, which would not match. If such a version is released, it will be incorrectly tagged aslatestinstead ofnext, overwriting stable releases.Fix to match any semantic versioning pre-release suffix:
Proposed fix
- if [[ $STRING == *"prerelease"* ]]; then + if [[ $STRING == *-* ]]; then
🧹 Nitpick comments (5)
docs/CNAME (1)
1-1: Unrelated change bundled in a CI-focused PR.This CNAME file for GitHub Pages (
docs.ad4m.dev) is unrelated to the npm-publish Docker container fix. Consider splitting it into a separate PR for cleaner git history and easier reverts..github/workflows/publish_staging.yml (1)
402-406:--no-frozen-lockfileweakens reproducibility in CI.Using
--no-frozen-lockfileallows pnpm to modify the lockfile during install, meaning the published artifacts may not match what was tested locally. This was likely added because the container's pnpm version differs from what generated the lockfile.If the mismatch is only a pnpm version issue, consider aligning the pnpm version in the container or using
--no-frozen-lockfileonly as a documented workaround with a comment explaining why..github/workflows/publish.yml (3)
393-400: Same fragile prerelease detection as inpublish_staging.yml.On the
mainbranch, it's worth questioning whether a prerelease version should ever be published. Ifmainis strictly for production releases, this check may be unnecessary — or it could be turned into a safeguard that fails the job instead of silently publishing tonext.Same concern as in the staging workflow: matching only the literal
"prerelease"substring is narrow.
402-406:--no-frozen-lockfileconcern applies here as well.Same as the staging workflow — this weakens reproducibility guarantees for production publishes to npm. A comment explaining the rationale would help future maintainers.
380-478: Near-identicalnpm-publishjob duplicated acrosspublish.ymlandpublish_staging.yml.The npm-publish job definition is almost identical between the two workflow files. Consider extracting it into a reusable workflow to reduce maintenance burden and avoid drift.
|
|
||
| - name: Install deps | ||
| run: pnpm install | ||
| run: pnpm install --no-frozen-lockfile |
There was a problem hiding this comment.
is --no-frozen-lockfile the right thing for release builds? I don't think so.
- Changed prerelease check to use *-* pattern to catch versions like 0.11.2-dev.4 - Removed docs/CNAME file (unrelated to CI changes) Addresses CodeRabbit feedback.
|
Addressed the CodeRabbit feedback:
The setup steps (Node.js, pnpm, Go, Deno) were already removed in the previous commit - the Docker container has everything pre-installed. Ready for re-review! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/publish.yml:
- Around line 402-406: The workflow uses pnpm install with --no-frozen-lockfile
in the "Install deps" and "Install core dependencies" steps which allows
publishing with untested dependency versions; change both steps to use the
CI-safe --frozen-lockfile flag (or remove the explicit flag to use the default
frozen behavior) and, if you intentionally need relaxed resolution, add a short
comment explaining why and where the lockfile will be updated instead of
allowing silent resolution differences.
Release builds should use exact tested dependencies from lockfile.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish.yml (1)
424-478:⚠️ Potential issue | 🟠 MajorUpgrade
JS-DevTools/npm-publish@v1to@v3or@v4.v1 runs on Node 12, which GitHub Actions removed from runners on August 14, 2023. These publish steps will fail on current runners. v4 is the current major version; v3 is also stable.
The breaking changes in v2+ do not affect this code: v1's
check-versionandgreater-version-onlyinputs were removed (the code doesn't use them), and output handling changed, but these particular publish steps only usetoken,package,tag, andaccess, which remain compatible. See the migration guide for details if you use other inputs.Example diff for one publish step
- name: Publish core - uses: JS-DevTools/npm-publish@v1 + uses: JS-DevTools/npm-publish@v3 with: token: ${{ secrets.COASYS_NPM_TOKEN }} package: core/package.json tag: ${{ env.NPM_TAG }} access: public
🧹 Nitpick comments (1)
.github/workflows/publish_staging.yml (1)
402-406: Consider using--frozen-lockfileto enforce reproducible builds.The commit message states the intent is reproducible builds by using exact lockfile dependencies, but plain
pnpm installcan still update the lockfile if versions drift. Use--frozen-lockfileto make the install fail ifpnpm-lock.yamlis out of sync, which is the standard CI best practice.♻️ Suggested diff
- name: Install deps - run: pnpm install + run: pnpm install --frozen-lockfile - name: Install core dependencies - run: cd ./core && pnpm install + run: cd ./core && pnpm install --frozen-lockfile
2 participants
Summary
Use the same Docker image that CircleCI uses for the npm-publish job in GitHub Actions.
Problem
The npm-publish job was failing with:
The
@mattrglobal/node-bbs-signaturespackage doesn't have pre-built binaries for Node 18 (v108 ABI).Solution
Use the
coasys/ad4m-ci-linuxDocker image (same as CircleCI) which has all dependencies pre-installed and configured.Changes
publish.yml: npm-publish job now uses Docker containerpublish_staging.yml: npm-publish job now uses Docker containerNotes
This uses the same pinned image SHA that CircleCI uses for consistency.
Summary by CodeRabbit