Skip to content
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

chore: factorize JDK specification to docker bakefile (linux) / compose file (windows) #409

Merged

Conversation

dduportal
Copy link
Contributor

@dduportal dduportal commented May 6, 2023

Blocked by #414

This pull request aims at factorizing the image definitions (Dockerfiles) to avoid duplicating code between JDK versions.

Until now, changing an element in any of the images which have both JDK11 and JDK17 definitions were subject to a risk of shift if the 2nd image is forgotten.

How does it work?

  • The directories 11/ and 17/, representing the major JDK version, are removed
  • There is only 1 directory per operating system at the root of the repository
    • Please note that bullseye is renamed to debian during this change for the following reasons:
  • The JDK version is passed through build arguments
    • The Dockerfiles do have a default version defined to ensure it's buildable "as it" (e.g. without Docker BuildX and its bake file). The proposed convention is to set it to the "most recent LTS JDK of the images"..
  • Archlinux only exists for JDK11. It's updated to follow the same rules, and adding a JDK17 version should be another PR (and will be needed AND a good test for this refactoring).

TODO:

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@timja
Copy link
Member

timja commented May 6, 2023

any way to see if anyone actually uses Archlinux?

@dduportal dduportal force-pushed the chore/factorize-Dockerfile-per-JDK branch 3 times, most recently from def8060 to 7f92859 Compare May 9, 2023 06:33
@dduportal
Copy link
Contributor Author

any way to see if anyone actually uses Archlinux?

The DockerHub download stats would be useful there

@dduportal dduportal force-pushed the chore/factorize-Dockerfile-per-JDK branch from 7f92859 to f62a799 Compare May 10, 2023 17:51
@dduportal
Copy link
Contributor Author

Rebased on top of #414 : let's see if it works as expected

@dduportal dduportal changed the title chore(linux) factorize JDK specification to docker bakefile chore: factorize JDK specification to docker bakefile (linux) / compose file (windows) May 10, 2023
@dduportal dduportal force-pushed the chore/factorize-Dockerfile-per-JDK branch from 33230e9 to 2dfd7b9 Compare May 13, 2023 20:58
@dduportal dduportal marked this pull request as ready for review May 13, 2023 21:04
@dduportal dduportal requested a review from a team as a code owner May 13, 2023 21:04
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Looks very good to me.

@dduportal dduportal merged commit bc9720f into jenkinsci:master May 14, 2023
@dduportal dduportal deleted the chore/factorize-Dockerfile-per-JDK branch May 14, 2023 07:25
dduportal added a commit that referenced this pull request May 17, 2023
chore(updatecli) fix JDK version tracking - fixup of #409
dduportal added a commit to dduportal/docker-agent that referenced this pull request May 17, 2023
dduportal added a commit that referenced this pull request May 17, 2023
chore: fix Dockerfile location on the last items - fixup of #409
@dduportal
Copy link
Contributor Author

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

Successfully merging this pull request may close these issues.

3 participants