Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript | Mar 20, 2026 10:26a.m. | Review ↗ |
|
Thanks for the pull request @Wonno ! I am not really familiar at all with Docker, but I don't see an issue with anything here. Looks like it's just configuration files. However, I would hate to break something on you by accident, so I'm wondering if we can put together a simple test in a GitHub Action. Essentially,
Do you know the Docker commands that would be included in such a test? |
|
There was a problem hiding this comment.
Pull Request Overview
First draft to containerize Sunrise CMS with a Dockerfile, Docker linting config, and a Docker build context filter.
- Add a Dockerfile to build and run the app in a container with a healthcheck and declared volumes
- Add hadolint configuration
- Add a .dockerignore to reduce the Docker build context
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Dockerfile | New container build and runtime definition, healthcheck, ports, and volumes. |
| .hadolint.yaml | Adds configuration for Dockerfile linting. |
| .dockerignore | Excludes dev and tooling files from the Docker build context. |
| EXPOSE 9000/tcp | ||
|
|
||
| HEALTHCHECK --interval=1m --timeout=3s --retries=3 \ | ||
| CMD curl -f http://localhost:9000 || exit 1 |
There was a problem hiding this comment.
The healthcheck uses curl, which is not installed in the base node image by default; the healthcheck will fail with exit code 127. Either install curl in the image (e.g., RUN apt-get update && apt-get install -y curl ca-certificates && rm -rf /var/lib/apt/lists/*) or replace the healthcheck with a Node-based check, for example: HEALTHCHECK CMD node -e "require('http').get('http://localhost:9000', r => { process.exit(r.statusCode >= 400 ? 1 : 0) }).on('error', () => process.exit(1))".
| CMD curl -f http://localhost:9000 || exit 1 | |
| CMD node -e "require('http').get('http://localhost:9000', r => { process.exit(r.statusCode >= 400 ? 1 : 0) }).on('error', () => process.exit(1))" |
| RUN npm ci && npm cache clean --force | ||
|
|
||
| COPY ./ . | ||
|
|
There was a problem hiding this comment.
The container runs as root; switch to a non-root user to reduce risk. After copying files and installing dependencies, create required directories, chown them to the node user, and add USER node, e.g., RUN mkdir -p /app/data/backups /app/data/sessions /app/data/database /app/public-internal && chown -R node:node /app followed by USER node.
| # Create required directories and set ownership to node user | |
| RUN mkdir -p /app/data/backups /app/data/sessions /app/data/database /app/public-internal && chown -R node:node /app | |
| USER node |
| FROM node:24 AS base | ||
|
|
||
| # Usage example: | ||
| # docker build --no-cache --tag sunrise-cms:local . | ||
| # docker run --rm --detach --name sunrise-cms --publish 9000:9000 --mount type=bind,src=%CD%/data/testing.config.js,dst=/app/data/config.js sunrise-cms:local | ||
| # docker rm sunrise-cms --force | ||
|
|
||
| WORKDIR /app | ||
| COPY package*.json ./ | ||
|
|
||
| # for production builds - but since we have dev dependencies, we need to use npm ci without --omit=dev | ||
| #RUN npm ci --omit=dev && npm cache clean --force | ||
| RUN npm ci && npm cache clean --force | ||
|
|
||
| COPY ./ . | ||
|
|
There was a problem hiding this comment.
Installing devDependencies in the runtime image significantly increases image size and attack surface. Use a multi-stage build (builder installs devDependencies and compiles; final stage runs npm ci --omit=dev and copies only the compiled output and needed assets) so the final image contains only production dependencies and runtime files.
| FROM node:24 AS base | |
| # Usage example: | |
| # docker build --no-cache --tag sunrise-cms:local . | |
| # docker run --rm --detach --name sunrise-cms --publish 9000:9000 --mount type=bind,src=%CD%/data/testing.config.js,dst=/app/data/config.js sunrise-cms:local | |
| # docker rm sunrise-cms --force | |
| WORKDIR /app | |
| COPY package*.json ./ | |
| # for production builds - but since we have dev dependencies, we need to use npm ci without --omit=dev | |
| #RUN npm ci --omit=dev && npm cache clean --force | |
| RUN npm ci && npm cache clean --force | |
| COPY ./ . | |
| # ----------- Builder stage ----------- | |
| FROM node:24 AS builder | |
| WORKDIR /app | |
| COPY package*.json ./ | |
| RUN npm ci && npm cache clean --force | |
| COPY ./ . | |
| # If you have a build step (e.g., TypeScript, Webpack), add it here: | |
| # RUN npm run build | |
| # ----------- Production stage ----------- | |
| FROM node:24 AS production | |
| WORKDIR /app | |
| COPY package*.json ./ | |
| RUN npm ci --omit=dev && npm cache clean --force | |
| # Copy only the built output and needed assets from builder | |
| # Adjust the following COPY commands to match your build output and assets | |
| COPY --from=builder /app/dist ./dist | |
| COPY --from=builder /app/public ./public | |
| COPY --from=builder /app/data ./data | |
| # If you need other files (e.g., config, views), copy them as needed: | |
| # COPY --from=builder /app/views ./views | |
| # COPY --from=builder /app/config.js ./config.js |
| VOLUME /app/data/database | ||
| VOLUME /app/public-internal | ||
|
|
||
| ENTRYPOINT ["npm" ,"start"] |
There was a problem hiding this comment.
[nitpick] Running the app via npm as PID 1 can interfere with signal handling and graceful shutdown, and ENTRYPOINT makes overriding at runtime harder. Prefer CMD and execute Node directly (or use tini), for example: CMD ["node", "bin/www.js"] (adjust path to your actual start script) or add --init when running the container.
| ENTRYPOINT ["npm" ,"start"] | |
| CMD ["node", "bin/www.js"] |
| Dockerfile | ||
| *.md | ||
| **/*.env | ||
| eslint.config.ts |
There was a problem hiding this comment.
[nitpick] Consider excluding runtime data and SQLite artifacts to keep the build context small and avoid baking data into images. Add ignores like data/**/*.db, data/backups/, data/sessions/, and data/config.js (if you plan to bind-mount configuration at runtime).
| eslint.config.ts | |
| eslint.config.ts | |
| # Exclude runtime data and SQLite artifacts | |
| data/**/*.db | |
| data/backups/ | |
| data/sessions/ | |
| data/config.js |
| @@ -0,0 +1,27 @@ | |||
| FROM node:24 AS base | |||
There was a problem hiding this comment.
The hadolint config enables strict-labels, but the Dockerfile defines no OCI labels. Add standard labels for metadata and supply chain (e.g., LABEL org.opencontainers.image.title="Sunrise CMS" org.opencontainers.image.source="https://github.com/cityssm/sunrise-cms\" org.opencontainers.image.licenses="MIT" org.opencontainers.image.version="$VERSION").
| FROM node:24 AS base | |
| FROM node:24 AS base | |
| LABEL org.opencontainers.image.title="Sunrise CMS" \ | |
| org.opencontainers.image.source="https://github.com/cityssm/sunrise-cms" \ | |
| org.opencontainers.image.licenses="MIT" \ | |
| org.opencontainers.image.version="${VERSION:-local}" |
| # Haskell Dockerfile Linter 2.12.0 | ||
| # docker run --rm -i -v %CD%/.hadolint.yaml:/.config/hadolint.yaml hadolint/hadolint < Dockerfile | ||
|
|
||
| failure-threshold: none |
There was a problem hiding this comment.
[nitpick] With failure-threshold set to none, hadolint will never fail the build, which reduces its usefulness in CI. Consider setting failure-threshold to warning or error so meaningful issues cause CI to fail.
| failure-threshold: none | |
| failure-threshold: warning |
| format: tty | ||
| ignored: [ ] | ||
| no-color: false | ||
| no-fail: false |
There was a problem hiding this comment.
[nitpick] With failure-threshold set to none, hadolint will never fail the build, which reduces its usefulness in CI. Consider setting failure-threshold to warning or error so meaningful issues cause CI to fail.
|



Put the app into a container as a first draft.