-
Notifications
You must be signed in to change notification settings - Fork 46
BUGFIX/HCMPRE-0010 #3009
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
base: master
Are you sure you want to change the base?
BUGFIX/HCMPRE-0010 #3009
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,12 @@ FROM node:14-alpine3.16 AS build | |
RUN apk update | ||
RUN apk add --no-cache 'git>2.30.0' | ||
ARG WORK_DIR | ||
ARG GIT_COMMIT=unknown | ||
ARG GIT_BRANCH=unknown | ||
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Build args for VCS metadata added — ensure CI supplies them and consider surfacing in final image. Good addition. Two follow-ups:
Proposed change to expose in the final stage (after the second FROM): FROM nginx:mainline-alpine
#FROM ghcr.io/egovernments/nginx:mainline-alpine
+ARG GIT_COMMIT=unknown
+ARG GIT_BRANCH=unknown
+ENV GIT_COMMIT=$GIT_COMMIT
+ENV GIT_BRANCH=$GIT_BRANCH
ENV WORK_DIR=/var/web/health-dss 🤖 Prompt for AI Agents
|
||
|
||
ENV GIT_COMMIT=$GIT_COMMIT | ||
ENV GIT_BRANCH=$GIT_BRANCH | ||
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) ENV set only in build stage won’t be present in the final image. If your React/Vue build pipeline consumes these during build, you’re fine. If you expect to read them at container runtime (served by nginx), re-declare them in the final stage as shown in my other comment. 🤖 Prompt for AI Agents
|
||
|
||
WORKDIR /app | ||
ENV NODE_OPTIONS="--max-old-space-size=4792" | ||
ENV YARN_DEBUG=true | ||
|
@@ -37,7 +43,7 @@ RUN yarn build:webpack | |
|
||
FROM nginx:mainline-alpine | ||
#FROM ghcr.io/egovernments/nginx:mainline-alpine | ||
ENV WORK_DIR=/var/web/dashboard-ui | ||
ENV WORK_DIR=/var/web/health-dss | ||
|
||
RUN mkdir -p ${WORK_DIR} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,12 @@ | ||||||||||||||||||||||||
server | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
listen 80; | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Harden vhost selection: set default_server and an explicit server_name. To avoid accidental vhost collisions if more server blocks exist, declare default_server and a catch-all server_name. - listen 80;
+ listen 80 default_server;
+ server_name _; 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||
underscores_in_headers on; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
location /dashboard-ui | ||||||||||||||||||||||||
{ | ||||||||||||||||||||||||
root /var/web; | ||||||||||||||||||||||||
index index.html index.htm; | ||||||||||||||||||||||||
try_files $uri $uri/ /dashboard-ui/index.html; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
Comment on lines
+6
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Optional: add caching headers for static assets and prevent caching index.html. For hashed bundle assets, long-lived caching improves performance. Avoid caching index.html to ensure users get the latest app shell. Add these sibling locations in the same server block (outside the existing location): # Long cache for static assets (adjust extensions to your build artifacts)
location ~* \.(?:js|css|png|jpg|jpeg|gif|svg|ico|woff2?|ttf|map)$ {
add_header Cache-Control "public, max-age=31536000, immutable";
expires 1y;
}
# Do not cache the SPA shell
location = /dashboard-ui/index.html {
add_header Cache-Control "no-store";
} 🤖 Prompt for AI Agents
💡 Verification agent 🧩 Analysis chainVerify docroot mapping: likely mismatch between Nginx root and Docker deployment path (may cause 404s). Given the Dockerfile change (AI summary) to deploy built assets under /var/web/health-dss, serving /dashboard-ui with Apply if assets are under /var/web/health-dss: - location /dashboard-ui
+ location ^~ /dashboard-ui/
{
- root /var/web;
+ alias /var/web/health-dss/;
index index.html index.htm;
try_files $uri $uri/ /dashboard-ui/index.html;
} Run this script to confirm where assets are copied in the final image: 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Searching Dockerfiles for health-dss workdir and copy destinations..."
fd -a Dockerfile | xargs -I{} sh -c 'echo "---- {} ----"; rg -n -C2 --no-heading -e "WORKDIR" -e "COPY" -e "/var/web" -e "health-dss" "{}"'
echo
echo "Confirming nginx.conf path(s) referenced and current content:"
fd -a nginx.conf | xargs -I{} sh -c 'echo "---- {} ----"; sed -n "1,120p" "{}"' Length of output: 15134 Fix Nginx docroot for health-dss assets The Dockerfile for • File: - location /dashboard-ui
- {
- root /var/web;
+ location ^~ /dashboard-ui/ {
+ alias /var/web/health-dss/;
index index.html index.htm;
try_files $uri $uri/ /dashboard-ui/index.html;
} This change maps 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||
} |
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.
🛠️ Refactor suggestion
Build failing: node-gyp can’t find Python (node-sass). Install python3, make, g++ and add python symlink.
The pipeline error indicates node-sass native compilation is failing due to missing Python. On Alpine, node-gyp requires python3 and build tools, and often a python symlink.
Apply this diff to fix the build:
Optional (if you hit further linking issues): add libc6-compat.
Note: You can also reduce layers by dropping the separate
apk update
since--no-cache
fetches fresh indexes.Also applies to: 39-39
🧰 Tools
🪛 Hadolint (2.12.0)
[warning] 3-3: Pin versions in apk add. Instead of
apk add <package>
useapk add <package>=<version>
(DL3018)
[info] 3-3: Multiple consecutive
RUN
instructions. Consider consolidation.(DL3059)
🤖 Prompt for AI Agents