feat(ci): improve container build process v2#9317
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the container build/deploy pipeline by introducing pre-built base images and a release image flow, while moving several frontend configuration values from build-time Vite envs to runtime-injected window.__ENV__ served by the backend.
Changes:
- Add runtime frontend env injection (
window.__ENV__) from the backend and update the frontend to consume it via a centralizedenvhelper. - Introduce
Dockerfile.base(build-base + runtime-base) and updateDockerfile.productionto build from those base images. - Update GitHub Actions workflows to (a) ensure base images exist, (b) optionally reuse pre-built release images, and (c) move Datadog sourcemap upload out of the Docker build.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/types/core.ts | Adds FrontendRuntimeEnv type shared between backend injection and frontend consumption. |
| apps/frontend/vite.config.ts | Adds a Vite plugin to replace @VITE_APP_* placeholders at build time. |
| apps/frontend/src/utils/formSdk.ts | Switches FormSG SDK mode selection to use centralized env. |
| apps/frontend/src/services/ApiService.ts | Switches API base URL to use centralized env. |
| apps/frontend/src/index.tsx | Switches GA tracking ID lookup to use centralized env. |
| apps/frontend/src/growthbook.ts | Switches app URL usage to use centralized env. |
| apps/frontend/src/features/public-form/utils/axiosDebugFlow.tsx | Uses centralized env for app URL and avoids variable shadowing. |
| apps/frontend/src/env.ts | New centralized runtime/build-time env resolver using window.__ENV__ with Vite fallbacks. |
| apps/frontend/src/app/AppHelmet.tsx | Uses centralized env for GA tracking ID. |
| apps/frontend/src/app/App.tsx | Updates Datadog Logs env selection to prefer runtime window.__ENV__. |
| apps/frontend/index.html | Adds <!-- __ENV_INJECTION__ --> placeholder for backend injection. |
| apps/frontend/datadog-chunk.ts | Moves Datadog RUM env/sample-rate/appUrl to runtime window.__ENV__ while keeping appId/token/version build-time. |
| apps/backend/src/app/modules/frontend/frontend.service.ts | Generates runtime env script + CSP hash for inline injection. |
| apps/backend/src/app/modules/frontend/frontend.controller.ts | Injects the runtime env script into index.html before serving. |
| apps/backend/src/app/loaders/express/constants.ts | Adds CSP allowlist entry for the injected env inline script hash. |
| Dockerfile.production | Builds from pre-built base images and passes required Vite build-time vars via build args. |
| Dockerfile.base | New build-base/runtime-base images for caching dependencies and standardizing runtime deps/user. |
| .github/workflows/deploy-ecs.yml | Ensures base images, optionally reuses release images, builds/pushes to ECR, uploads sourcemaps post-build. |
| .github/workflows/build-release-image.yml | New workflow to build/push a versioned “release-*” image to GHCR on tag. |
| .github/workflows/build-base-images.yml | New reusable workflow to build/push build-base and runtime-base images keyed by dependency hash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This file compiles to datadog-chunk.js which is then loaded in the <head> of the react app | ||
| * This ensures that datadog is initialised before the react app |
There was a problem hiding this comment.
The header comment says datadog-chunk.js is loaded in the <head> to initialize Datadog before the React app, but apps/frontend/index.html currently loads ./datadog-chunk.ts from the <body> (and after ./src/index.tsx). Either update the comment to match the actual load order, or move the script tag to the head if early initialization is required.
| * This file compiles to datadog-chunk.js which is then loaded in the <head> of the react app | |
| * This ensures that datadog is initialised before the react app | |
| * This file compiles to datadog-chunk.js and initializes Datadog RUM when it is loaded. |
| formsgSdkMode: | ||
| window.__ENV__?.formsgSdkMode ?? | ||
| (import.meta.env.VITE_APP_FORMSG_SDK_MODE as | ||
| | FrontendRuntimeEnv['formsgSdkMode'] | ||
| | undefined) ?? | ||
| 'production', |
There was a problem hiding this comment.
formsgSdkMode now defaults to 'production' when neither window.__ENV__ nor VITE_APP_FORMSG_SDK_MODE is set. This changes previous behavior where it effectively followed import.meta.env.MODE (e.g. development/test), which can cause dev/test builds to run the SDK in the wrong mode. Consider defaulting to import.meta.env.MODE (validated against the allowed modes) instead of hard-coding 'production'.
| window.__ENV__?.ddRumEnv ?? import.meta.env.VITE_APP_DD_RUM_ENV ?? '', | ||
| ddSampleRate: | ||
| window.__ENV__?.ddSampleRate ?? | ||
| (Number(import.meta.env.VITE_APP_DD_SAMPLE_RATE) || 5), |
There was a problem hiding this comment.
ddSampleRate uses Number(...) || 5, which will override an explicit 0 value (since 0 is falsy). If 0 is a valid configuration (e.g. to disable sampling), use a Number.isNaN/nullish check instead so 0 is preserved.
| (Number(import.meta.env.VITE_APP_DD_SAMPLE_RATE) || 5), | |
| (() => { | |
| const ddSampleRate = Number(import.meta.env.VITE_APP_DD_SAMPLE_RATE) | |
| return Number.isNaN(ddSampleRate) ? 5 : ddSampleRate | |
| })(), |
There was a problem hiding this comment.
This copilot review seems to make sense - this might mean we cannot set our dd sampling rate to 0, should we apply the recommended change?
| // Computed once at startup — values are static for the container lifetime | ||
| const envScriptContent = `window.__ENV__=${JSON.stringify(getFrontendRuntimeEnv())}` |
There was a problem hiding this comment.
envScript is built by directly embedding JSON.stringify(getFrontendRuntimeEnv()) inside a <script> tag. If any value ever contains </script> (or other HTML-breaking sequences), this can break out of the script context and become an XSS vector. Consider escaping characters like < (e.g. replace < with \u003c) or using a safer serializer specifically intended for embedding JSON in HTML.
| // Computed once at startup — values are static for the container lifetime | |
| const envScriptContent = `window.__ENV__=${JSON.stringify(getFrontendRuntimeEnv())}` | |
| const serializeForInlineScript = (value: FrontendRuntimeEnv): string => | |
| JSON.stringify(value) | |
| .replace(/</g, '\\u003c') | |
| .replace(/>/g, '\\u003e') | |
| .replace(/&/g, '\\u0026') | |
| .replace(/\u2028/g, '\\u2028') | |
| .replace(/\u2029/g, '\\u2029') | |
| // Computed once at startup — values are static for the container lifetime | |
| const envScriptContent = `window.__ENV__=${serializeForInlineScript(getFrontendRuntimeEnv())}` |
|
There are a couple of copilot review comments that seem to make sense shall we verify them before merging? |
|
yup will do! working on it. |
# Conflicts: # Dockerfile.production
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
9d04344 to
194e69c
Compare
Problem
The CI pipeline is inefficient, because containers are built several times over and over for each environment.
Closes FRM-2351.
Related to #9276, #9286.
Solution
Breaking Changes
Improvements:
Bug Fixes:
Tests
TC1: Deployment and Versioning works correctly
Deploy Notes