chore: Release CLI as a docker image #192
Conversation
WalkthroughAdds Docker packaging and CI release automation: a multi-stage Dockerfile and build script for single-/multi-platform images, CI steps to capture/compare package version and conditionally build/push Docker images on release, and documentation updates (README, HACKING) including Node 22+ requirement. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant ReleaseJob as Release Job
participant CIBuilder as CI Build (npm)
participant Script as scripts/build-docker.sh
participant Docker as Docker Engine / Buildx
participant Registry as Docker Hub
GH->>ReleaseJob: workflow triggered (release)
ReleaseJob->>CIBuilder: run npm build (produce dist)
CIBuilder-->>ReleaseJob: artifacts ready
ReleaseJob->>ReleaseJob: read package.json -> VERSION_before
ReleaseJob->>ReleaseJob: run release action
ReleaseJob->>ReleaseJob: read package.json -> VERSION_after
ReleaseJob->>ReleaseJob: compare -> set RELEASED
alt RELEASED == true
ReleaseJob->>Script: invoke build-docker.sh (env: VERSION, GITHUB_SHA, BUILD_DATE, DOCKER creds)
Script->>Docker: ensure daemon & buildx builder exists
Script->>Docker: build image(s) (single- or multi-arch)
alt push requested
Script->>Registry: docker login (if needed)
Docker->>Registry: push image(s) with labels
else load/local
Docker-->>ReleaseJob: load/tag image locally
end
Script-->>ReleaseJob: return status & tags
end
ReleaseJob->>GH: workflow completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
README.md (1)
26-40: Consider tightening the Docker alias/run docs (mounts + shell compatibility).
The alias assumes bash/zsh ($(pwd)) and Linux-style paths; also consider adding-itguidance (interactive commands) and a note that the mount is required for commands operating on local files.scripts/build-docker.sh (1)
137-180: Quote image references indocker push/images(edge-case safety).
These are currently unquoted expansions; unlikely to break in practice, but easy to harden.- docker push ${IMAGE_NAME}:${TAG} + docker push "${IMAGE_NAME}:${TAG}" @@ - docker push ${IMAGE_NAME}:${VERSION} + docker push "${IMAGE_NAME}:${VERSION}" @@ - docker images ${IMAGE_NAME}:${TAG} + docker images "${IMAGE_NAME}:${TAG}" @@ - docker images ${IMAGE_NAME}:${TAG} + docker images "${IMAGE_NAME}:${TAG}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release.yml(1 hunks)Dockerfile(1 hunks)HACKING.md(1 hunks)README.md(1 hunks)scripts/build-docker.sh(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/release.yml
32-32: shellcheck reported issue in this script: SC2086:info:1:66: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
50-51: Remove unnecessary chmod command.The
chmod +xis redundant since the entrypoint invokes the file via thenodeinterpreter (line 61:node ./dist/cli.js) rather than executing it directly. File execute permissions are not required when running through an interpreter.🔎 Apply this diff to remove the unnecessary command:
-# Make the CLI binary executable -RUN chmod +x ./dist/cli.js - # Create a non-root user for security
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🔇 Additional comments (5)
Dockerfile (5)
5-26: LGTM! Builder stage follows best practices.The builder stage correctly:
- Leverages Docker layer caching by copying package files first
- Uses
npm cifor reproducible builds- Cleans npm cache to reduce layer size
- Copies only necessary files for the build
28-38: LGTM! Production stage correctly installs only runtime dependencies.Using
npm ci --only=productionensures the production image includes only necessary runtime dependencies, keeping the image size minimal.
53-58: LGTM! Running as non-root user is a security best practice.Creating and switching to a non-root user prevents potential privilege escalation attacks and follows Docker security best practices.
60-64: LGTM! Entrypoint configuration follows Docker best practices.Using the exec form for
ENTRYPOINTwith a sensible defaultCMD(--help) provides a good user experience. Users can easily override the command to pass their own CLI arguments.
40-44: These files are correctly included and needed at runtime.The files
textmate/,extractor.d.ts, andschema.jsonare all runtime dependencies, not just build artifacts:
- schema.json: Used at runtime in
src/config/tolgeerc.tsto validate configuration via Ajv- textmate/: Used at runtime in
src/extractor/tokenizer.tsto load TextMate grammar files for syntax highlighting and code tokenization during extraction- extractor.d.ts: TypeScript definition file needed for proper type information at runtime
These files should remain in the production image.
JanCizmar
force-pushed
the
jancizmar/release-docker-image
branch
from
269ca08 to
5e88d34
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
54-61: Handle both workflow trigger types for SHA and timestamp.Lines 60-61 use
workflow_runevent context, which is only available when triggered byworkflow_run(line 3). When triggered bypushto thejancizmar/release-docker-imagebranch (line 9), these values will be null/undefined, causing the Docker labels to be incomplete or the script to fail.🔎 Apply this diff to handle both trigger types:
- name: Build and push Docker image run: ./scripts/build-docker.sh latest linux/amd64,linux/arm64 push env: DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} VERSION: ${{ steps.package-version.outputs.VERSION }} - GITHUB_SHA: ${{ github.event.workflow_run.head_sha }} - BUILD_DATE: ${{ github.event.workflow_run.head_commit.timestamp }} + GITHUB_SHA: ${{ github.event.workflow_run.head_sha || github.sha }} + BUILD_DATE: ${{ github.event.workflow_run.head_commit.timestamp || github.event.head_commit.timestamp }}
🧹 Nitpick comments (2)
scripts/build-docker.sh (1)
145-152: Quote variables indocker pushcommands.Lines 148 and 150 use unquoted variable expansions. While these variables are controlled and unlikely to contain special characters, quoting is a shell best practice for consistency and safety.
🔎 Apply this diff to quote the variables:
if [ "$PUSH" = "push" ] && [ -z "$PLATFORM" ]; then # For regular builds, we need to push separately echo -e "${GREEN}Pushing images to registry...${NC}" - docker push ${IMAGE_NAME}:${TAG} + docker push "${IMAGE_NAME}:${TAG}" if [ -n "${VERSION:-}" ] && [ "$TAG" = "latest" ]; then - docker push ${IMAGE_NAME}:${VERSION} + docker push "${IMAGE_NAME}:${VERSION}" fi fiDockerfile (1)
38-38: Consider using modern npm flag--omit=dev.The
--only=productionflag still works but--omit=devis the newer, preferred syntax in npm 7+ (which Node 24 includes). Both achieve the same result, but the newer flag is more explicit and future-proof.🔎 View suggested change:
# Install ONLY production dependencies -RUN npm ci --only=production && npm cache clean --force +RUN npm ci --omit=dev && npm cache clean --force
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release.yml(3 hunks)Dockerfile(1 hunks)HACKING.md(2 hunks)README.md(1 hunks)scripts/build-docker.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- HACKING.md
🧰 Additional context used
🪛 GitHub Actions: Release
scripts/build-docker.sh
[error] 1-1: Command './scripts/build-docker.sh latest linux/amd64,linux/arm64 push' failed: DOCKERHUB_USERNAME and DOCKERHUB_TOKEN environment variables are required for push.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Unit Tests (macos-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (5)
scripts/build-docker.sh (2)
43-56: LGTM! Docker and credentials validation is thorough.The validation logic correctly checks Docker availability and enforces required credentials for push operations. The pipeline failure indicates the workflow needs to ensure secrets are available, not a script issue.
64-143: Excellent use of arrays to build Docker commands safely.The array-based command construction (instead of eval) properly prevents injection issues and word-splitting problems, addressing the previous review feedback effectively.
.github/workflows/release.yml (1)
57-58: Verify Docker Hub secrets are configured.The pipeline failure indicates
DOCKERHUB_USERNAMEandDOCKERHUB_TOKENare not available. While the workflow correctly references these secrets, they may not be configured in the repository settings or may not be accessible to this branch.Please verify that:
DOCKERHUB_USERNAMEandDOCKERHUB_TOKENsecrets are configured in the repository settings- Branch protection or security settings aren't blocking secret access to the feature branch
- After fixing the workflow_run context issue (previous comment), test the workflow to confirm secrets are properly passed
Dockerfile (2)
1-64: Excellent multi-stage Docker build with security best practices.The Dockerfile follows best practices:
- Multi-stage build minimizes final image size
- Proper layer caching with package files first
- Production stage includes only runtime dependencies
- Non-root user execution for security
- Clear entrypoint and default command
3-3: No changes needed. Node.js 24, codenamed Krypton, was released on May 6, 2025, and Node.js 24.11.0 entered Long-Term Support (LTS) and will continue receiving maintenance and security updates through April 2028. Thenode:24-alpineimage is stable, available, and appropriate for both build and production stages. Dependencies are compatible with Node.js 24.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
🔇 Additional comments (3)
.github/workflows/release.yml (3)
41-43: LGTM: Version extraction uses correct quoting.The quoting issue from previous reviews has been addressed. Using single quotes for the Node.js expression prevents shell parsing conflicts.
54-56: LGTM: Post-release version extraction is correct.Consistent with the pre-release extraction and properly quoted.
58-67: LGTM: Version comparison logic is sound.The comparison correctly determines whether a new version was released and provides clear logging for debugging.
|
🎉 This PR is included in version 2.15.1-rc.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.15.1-rc.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Anty0
left a comment
Anty0
left a comment
There was a problem hiding this comment.
The build-docker.sh has a lot of "maintainability" issues, but I guess it's fine since it's just a build script. It's very hard to read, but I didn't find any issues when reviewing it.
| **Basic build (current platform only):** | ||
|
|
||
| ```bash | ||
| ./scripts/build-docker.sh |
There was a problem hiding this comment.
Could the command be added to scripts in package.json? That way people would find it IMO easier.
There was a problem hiding this comment.
Hmm, thanks for the feedback.
I was thinking about writing it in JS so it's multiplatform. But I am not sure about running bash scripts via npm run... AFAIK, it can break on Windows...
I don't think it's something devs would use often and if they search for it it's in the docs... 🤷♂️
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E Tests (ubuntu-latest, 18)
- GitHub Check: E2E Tests (ubuntu-latest, 22)
- GitHub Check: E2E Tests (ubuntu-latest, 20)
🔇 Additional comments (3)
Dockerfile (3)
1-26: Excellent multi-stage build approach for the builder stage.The builder stage follows Docker best practices: package files are copied first for optimal layer caching, all dependencies (including dev dependencies needed for building) are installed, and all necessary source files are copied before the build step.
39-63: Strong security posture in the production stage.The production stage demonstrates good security practices: it copies only the necessary runtime artifacts (no source code), creates a non-root user with a specific UID/GID for better permission management, and switches to that user before setting the entrypoint. The executable permissions and default help command are appropriately configured.
3-3: Node.js 24 is production-ready and recommended for use.Node.js 24 entered Active LTS on October 28, 2025. Production applications should only use Active LTS or Maintenance LTS releases, and most production systems should be on Node.js 24. The use of
node:24-alpineon lines 3 and 28 is appropriate and aligns with current Node.js best practices.
|
🎉 This PR is included in version 2.15.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.