Moved packages from exit script to build image creation #2742
Conversation
|
|
||
| function on_exit { | ||
| apk add --no-cache procps | ||
| apk add --no-cache coreutils |
There was a problem hiding this comment.
These are the two packages that we want to get installed earlier during Docker image creation.
There was a problem hiding this comment.
Pull request overview
This PR improves Docker image immutability and Celery shutdown performance by moving the installation of procps and coreutils packages from the runtime exit script into the Docker image build process.
- Removed dynamic package installation from
run_celery_exit.sh - Added
procpsandcoreutilsto Alpine-based Dockerfiles (ci/Dockerfileandci/Dockerfile.test) - Added
coreutilsto the Debian-based devcontainer Dockerfile
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/run_celery_exit.sh | Removed runtime installation of procps and coreutils packages from the exit handler |
| ci/Dockerfile | Added procps and coreutils to the Alpine package installation list and improved formatting |
| ci/Dockerfile.test | Added procps and coreutils to the Alpine package installation list and improved formatting |
| .devcontainer/Dockerfile | Added coreutils to the Debian package installation list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| coreutils \ | ||
| procps \ |
There was a problem hiding this comment.
The package ordering is inconsistent between the two Alpine Dockerfiles. In ci/Dockerfile, procps appears before coreutils (lines 12-13), while in ci/Dockerfile.test, coreutils appears before procps (lines 15-16). For better maintainability and consistency, these packages should be ordered the same way in both files. Consider using alphabetical ordering for all packages to make the lists easier to maintain and compare.
| coreutils \ | |
| procps \ | |
| procps \ | |
| coreutils \ |
.devcontainer/Dockerfile
Outdated
| && apt-get -y install \ | ||
| 2>&1 && \ | ||
| apt-get -y install \ | ||
| coreutils \ |
There was a problem hiding this comment.
The coreutils package is pre-installed by default in Debian-based images (including the mcr.microsoft.com/vscode/devcontainers/python base image). Explicitly adding it to the package list is redundant and unnecessary. Consider removing this line as it doesn't add any value and may cause confusion about why it's being explicitly installed.
| coreutils \ |
whabanks
left a comment
whabanks
left a comment
There was a problem hiding this comment.
LGTM, great catch maintaining immutability post-build 🚀.
…uld be included by default in debian
* adding workflows for fresh deploys to dev from 'dev' branch * updates for names and removing ghost code. from staging workflow * new workflow to handle building and deploying images to our dev ECR on dev branch changes * Delete .github/workflows/lambda_staging.yml we dont need this * adding trigger for manifest workflow * adding back this file to state it was in main * fixintg * adding latest tags * updating image names for consistency * adding workflows to build and push and tag accordingly to all 3 ECRs using the new roles in TF * using var for the branch in the role name * same for role-session-name * tightening this up, and calling manifests update docker tag * calling the right workflow * just commenting out prod for this until it's released and working * setting up secrets properly * removing matrix to get this working * fixing secrets reference * need to upload to manage the image * testing against dev and stagin, removing latest tags * simplifying to only push sha tag, and skips if exiswts * turning back on the workflow call in manifests * removing extra argument * updating some names * just making it dev for now * just updating comments * nothing * updating token * nothing the token just needed to be updated * better error handling * tweaks * adding dev back * just tweaking * bringing back the stuff * whoops * nothing * nothing2 * removing comments * Trigger workflow: empty commit * Trigger workflow: empty commit * adding latest tag * rolling back * Trigger workflow: empty commit * Trigger workflow: empty commit * Trigger workflow: empty commit * QA test (#2737) * Added a Monthly Notification Stats summary table (#2734) * added a summary table * Update migrations/versions/0500_add_materialized_view.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * QA test --------- Co-authored-by: Jumana B <jzbahrai@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ben Larabie <ben.larabie@blarabie-jtfk.home> * Moved packages from exit script to build image creation (#2742) * Moved packages from exit script to build image creation * Removed coreutils from the devcontainer package installation; this would be included by default in debian * Ordered package to be the same across Dockerfile files --------- Co-authored-by: Michael Pond <mikepond11@gmail.com> Co-authored-by: Mike Pond <32133001+P0NDER0SA@users.noreply.github.com> Co-authored-by: Ben Larabie <benlarabie@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ben Larabie <ben.larabie@blarabie-jtfk.home> Co-authored-by: Jimmy Royer <jimleroyer@gmail.com>
Summary | Résumé
Moved package installation from the
run_celery_exit.shscript into the image creation. This makes the image safer as the images should be immutable after creation and the celery exit script should be faster as well, getting away from installing package on last minute.Related Issues | Cartes liées
Test instructions | Instructions pour tester la modification
Make sure the Celery instances shutdown properly.
Release Instructions | Instructions pour le déploiement
Make sure the Celery instances shutdown properly.
Reviewer checklist | Liste de vérification du réviseur