-
Notifications
You must be signed in to change notification settings - Fork 5
Replace cimg base image with ubuntu:latest #95
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
Conversation
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.
General Docker changes look good to me. Not sure how to answer the question about pip
and --break-system-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.
I believe the built image is missing a few tools used in the scripts here and there in dd-trace-java.
Got a few questions / remarks though, that may be addressed later.
-
I noticed the versioning is based on year + month
dd-trace-java-docker-build/build
Line 212 in 3c26d22
version="$(date +%y.%m)" Since this is like a major change shall we label this differently? That said this could affect other part of the build pipelines.
-
Circle CI image appear to use the
circleci
user, this requires commands to be run as sudo. Should we reproduce this behavior?
3763033
to
bdee917
Compare
@bric3 What do you mean by labelling differently? From this PR, it seems like the intention is to label each image by when it was last pulled, so year/month seems to make sense 🤔 |
@bric3 Good point! Looking into this, it seems like it is good security practice for the Dockerfile to run as a non-root user (one ref). However, we can still install everything at the root without using the |
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.
Looking good.
Few comments about cleaning it up further:
- Is the
yq
,docker
anddocker-compose
still needed? I used to install them manually to override the outdated version from the CircleCI image but I don't know if they are used. - Similarly I don't know if autoforward is still used 🤷
- And I ask to @smola what
ubuntu17
is for and if it is still relevant
Do you know how to test the image from your PR in the CI to test it?
Dockerfile
Outdated
sudo cp -rf --remove-destination /etc/java-17-openjdk/* /usr/lib/jvm/ubuntu17/lib/ | ||
sudo cp -f --remove-destination /etc/java-17-openjdk/jvm-amd64.cfg /usr/lib/jvm/ubuntu17/lib/ | ||
apt-get update | ||
apt-get install -y openjdk-17-jdk |
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 might want to revisit the cron time too: |
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.
For the tags, it's just that the current image inherit cimg
, while this PR changes it to ubuntu, I pondered on making the change of the base image more visible via tags.
0144d54
to
a485235
Compare
Testing the image build with |
f0d76bd
to
727afce
Compare
🎉 |
With our transition from CircleCI to Gitlab, we want to replace the
cimg
base image with the smaller, faster, saferubuntu:24.04
image.