-
Notifications
You must be signed in to change notification settings - Fork 446
Optimize docker layers #1684
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
base: main
Are you sure you want to change the base?
Optimize docker layers #1684
Conversation
I used a play-scala-seed as the basis for my scripted test. It's not going to work on sbt 2.x because Play is only published for sbt 1.x. CI calls this command: Line 129 in b803b49
I wonder if there's an easy way to exclude |
Awesome work already @dwickern . I'll try to find some time to review this.
I don't recall any 😢 My first naive approach would be to not use play as the seed 😅 |
1f8864b
to
7482062
Compare
cce68af
to
8e8ca41
Compare
The error was trying to resolve Play's sbt plugin for sbt 2.0.0-M3, which doesn't exist
I created a
Play support for sbt 2.x is being tracked in playframework/playframework#12898. |
caff5e6
to
6050b88
Compare
fab755c
to
a921e4f
Compare
I've been banging my head against this and I think I finally know what's happening. We run scripted tests by publishing sbt-native-packager version |
We need to fetch tags in order for sbt-dynver to detect the version, otherwise the version defaults to 0.0.0-SNAPSHOT. This causes the `test-layer-groups-playframework` scripted test to fail because Play Framework depends on a newer version of sbt-native-packager. See https://github.com/sbt/sbt-dynver#setup
According to the comment on the previous line
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.
Feel free to merge and release @dwickern ❤️
case (_, path) if path.startsWith(jreDir) => 10 | ||
case (file, _) if dependencies(file) => 20 | ||
case (_, path) if path.startsWith(confDir) => 30 | ||
case (_, path) if path.startsWith(libDir) => 40 | ||
case (_, path) if path.startsWith(binDir) => 40 |
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.
This is the gist of this change, right? LGTM 🥳
I numbered the layers 10, 20, 30, 40 instead of 1, 2, 3, 4 so that it's easier to sandwich something in between the existing layers. For example, I build multiple docker images that have some dependencies in common. This will allow me to move those dependencies into their own layer between 10 and 20, without having to re-number all the layers.
The main fix here is to make sure project artifacts are not being mixed with libraryDependencies. This was an issue with Play Framework in particular because they have some non-standard artifacts (the -assets and -sans-externalized jars). Previously we had a special case for the classpath jar but not the launcher jar. Now we consider anything that's not an external dependency to be a project artifact.
Layer 10 - jlink JRE
Layer 20 - external dependencies
Layer 30 - conf directory
Layer 40 - project artifacts
Fixes #1677
This PR builds on prior work in #1425 and #1326