Skip to content

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

Merged
merged 15 commits into from
May 22, 2025

Conversation

sarahchen6
Copy link
Contributor

@sarahchen6 sarahchen6 commented May 13, 2025

With our transition from CircleCI to Gitlab, we want to replace the cimg base image with the smaller, faster, safer ubuntu:24.04 image.

Copy link

@delner delner left a 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.

@sarahchen6 sarahchen6 marked this pull request as ready for review May 14, 2025 19:38
@sarahchen6 sarahchen6 requested a review from bric3 May 14, 2025 19:39
Copy link
Contributor

@bric3 bric3 left a 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.

  1. I noticed the versioning is based on year + month

    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.

  2. Circle CI image appear to use the circleci user, this requires commands to be run as sudo. Should we reproduce this behavior?

@sarahchen6 sarahchen6 force-pushed the sarahchen6/change-base-image branch from 3763033 to bdee917 Compare May 14, 2025 22:11
@sarahchen6
Copy link
Contributor Author

sarahchen6 commented May 14, 2025

Since this is like a major change shall we label this differently? That said this could affect other part of the build pipelines.

@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 🤔

@sarahchen6
Copy link
Contributor Author

sarahchen6 commented May 15, 2025

Circle CI image appear to use the circleci user, this requires commands to be run as sudo. Should we reproduce this behavior?

@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 sudo command and just switch to a non-root user during runtime at the end.

Copy link
Contributor

@PerfectSlayer PerfectSlayer left a 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 and docker-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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smola Do you remember why we added the ubuntu17 variant in the first place in #56 ?
Is there some difference with Temurin / OpenJDK 17?

@PerfectSlayer
Copy link
Contributor

You might want to revisit the cron time too:
https://github.com/DataDog/dd-trace-java-docker-build/blob/master/.github/workflows/ci.yml#L10-L12

@sarahchen6 sarahchen6 changed the title Replace cimg base image with ubuntu:24.04 Replace cimg base image with ubuntu:latest May 16, 2025
Copy link
Contributor

@bric3 bric3 left a 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.

@sarahchen6 sarahchen6 force-pushed the sarahchen6/change-base-image branch 2 times, most recently from 0144d54 to a485235 Compare May 19, 2025 12:58
@sarahchen6
Copy link
Contributor Author

Testing the image build with dd-trace-java PR: DataDog/dd-trace-java#8841

@sarahchen6 sarahchen6 force-pushed the sarahchen6/change-base-image branch from f0d76bd to 727afce Compare May 20, 2025 14:41
@sarahchen6 sarahchen6 merged commit b5634ed into master May 22, 2025
3 checks passed
@bric3
Copy link
Contributor

bric3 commented May 22, 2025

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants