-
Notifications
You must be signed in to change notification settings - Fork 4
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
delner
left a comment
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.
bric3
left a comment
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
circleciuser, 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 |
PerfectSlayer
left a comment
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,dockeranddocker-composestill 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
ubuntu17is 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: |
bric3
left a comment
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
cimgbase image with the smaller, faster, saferubuntu:24.04image.