Conversation
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds deploy-time service selection so staging/production workflows can build/push and roll out different service images (frontend and Temporal workers) using a parameterized Docker build action.
Changes:
- Add
serviceinput to staging/production deploy workflows and map it to dockerfile/image/deployment/container settings. - Generalize the
build-docker-imagecomposite action to acceptdockerfile,image-name, and optionalbuild-args. - Add a Dockerfile for the
package_downloads_workerservice (and a minor.dockerignoretweak).
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
workers/temporal/package_downloads_worker/Dockerfile |
Introduces a worker image build (multi-stage Node/pnpm install). |
workers/temporal/package_downloads_worker/.dockerignore |
Minor ignore list formatting change (note: build context currently makes this ineffective). |
.github/workflows/staging-deploy.yaml |
Adds service selector and deploys selected Kubernetes deployment/container. |
.github/workflows/production-deploy.yaml |
Same as staging; also gates Redis flush to frontend only. |
.github/actions/build-docker-image/action.yaml |
Makes Docker build/push reusable across services by parameterizing dockerfile/image name and build args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NUXT_REDIS_URL: ${{ env.NUXT_REDIS_URL }} | ||
| dockerfile: ${{ steps.config.outputs.dockerfile }} | ||
| image-name: ${{ steps.config.outputs.image_name }} | ||
| build-args: ${{ inputs.service == 'frontend' && format('NUXT_REDIS_URL={0}', env.NUXT_REDIS_URL) || '' }} |
There was a problem hiding this comment.
NUXT_REDIS_URL is being passed into the Docker build as a build-arg. Since frontend/Dockerfile exports it as an ENV during the build, this value (often including credentials) can be baked into image layers/build output and be recoverable from the image. Prefer injecting Redis configuration at runtime (Kubernetes env/config) rather than at build time, or ensure the build does not persist secrets into the resulting image.
| build-args: ${{ inputs.service == 'frontend' && format('NUXT_REDIS_URL={0}', env.NUXT_REDIS_URL) || '' }} | |
| build-args: '' |
| NUXT_REDIS_URL: ${{ env.NUXT_REDIS_URL }} | ||
| dockerfile: ${{ steps.config.outputs.dockerfile }} | ||
| image-name: ${{ steps.config.outputs.image_name }} | ||
| build-args: ${{ inputs.service == 'frontend' && format('NUXT_REDIS_URL={0}', env.NUXT_REDIS_URL) || '' }} |
There was a problem hiding this comment.
NUXT_REDIS_URL is being passed into the Docker build as a build-arg. Since frontend/Dockerfile exports it as an ENV during the build, this value (often including credentials) can be baked into image layers/build output and be recoverable from the image. Prefer injecting Redis configuration at runtime (Kubernetes env/config) rather than at build time, or ensure the build does not persist secrets into the resulting image.
| build-args: ${{ inputs.service == 'frontend' && format('NUXT_REDIS_URL={0}', env.NUXT_REDIS_URL) || '' }} | |
| build-args: '' |
There was a problem hiding this comment.
This one might be a security issue
| case "${{ inputs.service }}" in | ||
| frontend) | ||
| echo "dockerfile=frontend/Dockerfile" >> $GITHUB_OUTPUT | ||
| echo "image_name=insights-app" >> $GITHUB_OUTPUT | ||
| echo "k8s_deployment=insights-app-dpl" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
The case over ${{ inputs.service }} has no default (*) branch. If this workflow is triggered with an unexpected value (e.g., via API), it will continue with empty outputs and fail later in a less obvious way. Consider adding a default branch that prints an error and exits non-zero.
| case "${{ inputs.service }}" in | ||
| frontend) | ||
| echo "dockerfile=frontend/Dockerfile" >> $GITHUB_OUTPUT | ||
| echo "image_name=insights-app" >> $GITHUB_OUTPUT | ||
| echo "k8s_deployment=insights-app-dpl" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
The case over ${{ inputs.service }} has no default (*) branch. If this workflow is triggered with an unexpected value (e.g., via API), it will continue with empty outputs and fail later in a less obvious way. Consider adding a default branch that prints an error and exits non-zero.
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: . | ||
| file: frontend/Dockerfile | ||
| file: ${{ inputs.dockerfile }} | ||
| push: true |
There was a problem hiding this comment.
docker/build-push-action is invoked with context: ., which uses the repo-root .dockerignore. That .dockerignore currently excludes workers and submodules, but the worker Dockerfiles copy those paths, so builds for the worker services will fail with missing files. Consider updating the root .dockerignore to include these directories (or make the build context/dockerignore configurable per service) so worker builds have access to required sources.
There was a problem hiding this comment.
Not sure if this is a valid comment, but might be worth checking out
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
emlimlf
left a comment
There was a problem hiding this comment.
There are copilot comments that might pose a security risk please check them out
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: . | ||
| file: frontend/Dockerfile | ||
| file: ${{ inputs.dockerfile }} | ||
| push: true |
There was a problem hiding this comment.
Not sure if this is a valid comment, but might be worth checking out
| NUXT_REDIS_URL: ${{ env.NUXT_REDIS_URL }} | ||
| dockerfile: ${{ steps.config.outputs.dockerfile }} | ||
| image-name: ${{ steps.config.outputs.image_name }} | ||
| build-args: ${{ inputs.service == 'frontend' && format('NUXT_REDIS_URL={0}', env.NUXT_REDIS_URL) || '' }} |
There was a problem hiding this comment.
This one might be a security issue
| NUXT_REDIS_URL: ${{ env.NUXT_REDIS_URL }} | ||
| dockerfile: ${{ steps.config.outputs.dockerfile }} | ||
| image-name: ${{ steps.config.outputs.image_name }} | ||
| build-args: ${{ inputs.service == 'frontend' && format('NUXT_REDIS_URL={0}', env.NUXT_REDIS_URL) || '' }} |
Signed-off-by: Gašper Grom <gasper.grom@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
workers/temporal/package_downloads_worker/Dockerfile:1
- The builder stage is Node 24 but the runtime stage is Node 20. This can lead to runtime failures (e.g., dependency engines/ABI mismatches) since node_modules are installed under Node 24 and then copied into a Node 20 image. Align the Node major version between builder and runner (ideally parameterize via an ARG like the search-volume-worker Dockerfile).
workers/temporal/package_downloads_worker/Dockerfile:18 - The runtime stage uses
node:20-bookworm-slimwhile the builder usesnode:24-alpine. Copyingnode_modulesbuilt under a different Node major into this image is likely to break at runtime. Update this stage to use the same Node major as the builder (or revert builder to Node 20 if that’s the intended runtime).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.