-
Notifications
You must be signed in to change notification settings - Fork 2k
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
gha: switch to Ubuntu 24.04 #5139
Conversation
LOL that .. didn't go well; is
|
Hm... other checks use docker as well, so perhaps it's just the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5139 +/- ##
==========================================
- Coverage 61.82% 61.79% -0.04%
==========================================
Files 298 295 -3
Lines 20730 20725 -5
==========================================
- Hits 12817 12806 -11
- Misses 7000 7003 +3
- Partials 913 916 +3 |
Looks like it may be just the |
8d432cc
to
20a42ab
Compare
.github/workflows/e2e.yml
Outdated
if [ ! -f /tmp/foo.txt ]; then | ||
# ubuntu 24.04 runners no longer have a default daemon.json present | ||
sudo mkdir -p /etc/docker/ | ||
echo '{"experimental": true}' | sudo tee /etc/docker/daemon.json | ||
else | ||
# but if there is one; let's patch it to keep other options that may be set. | ||
sudo jq '.experimental = true' < /etc/docker/daemon.json > /tmp/docker.json | ||
sudo mv /tmp/docker.json /etc/docker/daemon.json | ||
fi |
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.
So... hm... it looks like CI was green even if we didn't enable experimental.
I'm wondering now if there's any (e2e) tests that require experimental 🤔 the only "SKIP" I found was in docker build
to test the --squash
flag, but I think we no longer gate that option by "experimental" (i.e., experimental only in wording?)
Line 128 in 591bd17
environment.SkipIfNotExperimentalDaemon(t) |
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.
Oh; never mind; we do: at least on the client side, but need to check what's on the daemon side
cli/cli/command/image/build.go
Lines 151 to 153 in 591bd17
flags.BoolVar(&options.squash, "squash", false, "Squash newly built layers into a single new layer") | |
flags.SetAnnotation("squash", "experimental", nil) | |
flags.SetAnnotation("squash", "version", []string{"1.25"}) |
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.
OK; still is gated on the daemon side https://github.com/moby/moby/blob/9d9488468fe2e08394a8b2fb8ad7fcfba34997f1/api/server/router/build/build_routes.go#L260-L262
However, if that's the ONLY test that differs, perhaps we should combine things; we can always enable experimental on the daemon (we already do).
This should be ready for review @krissetto @crazy-max I opened a separate ticket to look if we indeed still need the |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
20a42ab
to
3ce3177
Compare
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.
left a minor comment, but the rest LGTM
sudo jq '.experimental = true' < /etc/docker/daemon.json > /tmp/docker.json | ||
sudo mv /tmp/docker.json /etc/docker/daemon.json | ||
if [ ! -f /etc/docker/daemon.json ]; then | ||
# ubuntu 24.04 runners no longer have a default daemon.json present |
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.
If we're sure about this, do we want to keep this check just for safety in case they add it back? We might want to fail explicitly in those scenarios, just so we can notice the change and see if any values could create issues/are used incorrectly etc
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.
Yeah, that was my thinking; if they decide to set some options, we wouldn't immediately break.
But the cat /etc/docker/daemon.json
below should show us what's in there, in case their changes would cause a change in behavior and we wanted to see "what changed?"
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.
Yeah, i noticed that..I guess that can work, no hard feelings here anyway :)
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.
Thanks! Yeah, I guess it's one of those cases where there's pros/cons to either approach.
Let me bring this one in; thanks for reviewing!
- A picture of a cute animal (not mandatory but encouraged)