-
Notifications
You must be signed in to change notification settings - Fork 46
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
Update Makefile.docker-base #965
base: main
Are you sure you want to change the base?
Conversation
The changes include switching to a slimmer base image (python:3.11-slim), combining RUN commands, and adding cleanup steps to reduce the final image size. Additionally, a multi-stage build was implemented to ensure only necessary runtime files are included, enhancing efficiency and maintainability.
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.
Thanks for the PR, adding the comments to each of the pieces should help people reading the file.
Your PR description mentions changing the base image and a multi-stage build. I don't see any of those steps in here (I expected to see changes to docker-base/Dockerfile.buildx. Did you intend to have a different PR description?
groupadd --gid 1000 app | ||
useradd -d /app --uid 1000 --gid app app | ||
chown -R app:app /app | ||
|
||
apt-setup: | ||
# Configure APT to keep downloaded packages |
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.
because we cache them as part of the build so want them to be retained.
# gcc and python3-dev needed on arm for guidance | ||
DEBIAN_FRONTEND=noninteractive apt -y install --no-install-recommends python3-poetry gcc python3-dev | ||
# Install necessary packages without recommended packages | ||
DEBIAN_FRONTEND=noninteractive apt update && \ |
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.
You don't need to do the && thing here.
make will run both of the steps and will abort if the previous one fails.
rm -f /etc/apt/apt.conf.d/docker-clean | ||
echo 'Binary::apt::APT::Keep-Downloaded-Packages "true";' > /etc/apt/apt.conf.d/keep-cache | ||
|
||
apt-install: | ||
DEBIAN_FRONTEND=noninteractive apt update | ||
# gcc and python3-dev needed on arm for guidance |
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.
Please don't lose this comment. It's very important to understand why we're installing gcc & python3-dev (which otherwise we wouldn't expect to need).
# Install necessary packages without recommended packages | ||
DEBIAN_FRONTEND=noninteractive apt update && \ | ||
DEBIAN_FRONTEND=noninteractive apt -y install --no-install-recommends python3-poetry gcc python3-dev && \ | ||
# Clean up to reduce image size |
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.
We don't want this because of the caching. From docker-base/Dockerfile.buildx
RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \
--mount=type=cache,target=/var/lib/apt,sharing=locked \
make -f Makefile.docker-base apt-install
The first two lines are caching the directories, so they don't show up in the image.
You can verify that on the existing containers:
% docker run -it -u root arynai/sycamore-base bash
root@9983ae28df15:/app# find /var/cache/apt
/var/cache/apt
/var/cache/apt/archives
/var/cache/apt/archives/lock
/var/cache/apt/archives/partial
root@9983ae28df15:/app# find /var/lib/apt
/var/lib/apt
/var/lib/apt/mirrors
/var/lib/apt/mirrors/partial
/var/lib/apt/periodic
/var/lib/apt/extended_states
/var/lib/apt/lists
test "$(GIT_COMMIT)" != "" | ||
test "$(GIT_COMMIT)" != "unknown" | ||
touch .git.commit.$(GIT_COMMIT) | ||
|
||
# Allow images that depend on the docker base image to verify that the version for their |
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.
Please don't get rid of the longer comment that explains why we are doing this.
Feel free to move it under the target for consistency, but readers of the code need to understand what version consistency is being checked and why.
@@ -1,35 +1,40 @@ | |||
# -*- makefile -*- |
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.
Please don't get rid of this line. It's magic that helps some editors.
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.
Thanks for your reply.
I will check all this
The changes include switching to a slimmer base image (python:3.11-slim), combining RUN commands, and adding cleanup steps to reduce the final image size. Additionally, a multi-stage build was implemented to ensure only necessary runtime files are included, enhancing efficiency and maintainability.