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

gha: switch to Ubuntu 24.04 #5139

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Conversation

thaJeztah
Copy link
Member

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
Copy link
Member Author

LOL that .. didn't go well; is docker not installed by default on 24.04 runners?

Run sudo jq '.experimental = true' < /etc/docker/daemon.json > /tmp/docker.json
  sudo jq '.experimental = true' < /etc/docker/daemon.json > /tmp/docker.json
  sudo mv /tmp/docker.json /etc/docker/daemon.json
  sudo cat /etc/docker/daemon.json
  sudo service docker restart
  docker version
  docker info
  shell: /usr/bin/bash -e {0}
/home/runner/work/_temp/5a389600-a748-4fcd-a8a9-ac63e71c765e.sh: line 1: /etc/docker/daemon.json: No such file or directory
Error: Process completed with exit code 1.

@thaJeztah
Copy link
Member Author

Hm... other checks use docker as well, so perhaps it's just the daemon.json that's missing

@thaJeztah thaJeztah marked this pull request as draft June 11, 2024 11:05
@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.79%. Comparing base (591bd17) to head (3ce3177).

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     

@thaJeztah
Copy link
Member Author

Looks like it may be just the daemon.json that's missing; commenting-out that part looks like it works

Comment on lines 43 to 51
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
Copy link
Member Author

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?)

environment.SkipIfNotExperimentalDaemon(t)

Copy link
Member Author

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

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"})

Copy link
Member Author

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).

@thaJeztah thaJeztah marked this pull request as ready for review June 11, 2024 11:40
@thaJeztah thaJeztah requested review from krissetto and crazy-max June 11, 2024 11:46
@thaJeztah
Copy link
Member Author

This should be ready for review @krissetto @crazy-max

I opened a separate ticket to look if we indeed still need the experimental checks, or if they're really redundant;

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Copy link
Contributor

@krissetto krissetto left a 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
Copy link
Contributor

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

Copy link
Member Author

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?"

Copy link
Contributor

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 :)

Copy link
Member Author

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!

@thaJeztah thaJeztah merged commit 0022fe7 into docker:master Jun 11, 2024
88 checks passed
@thaJeztah thaJeztah deleted the gha_ubuntu_2404 branch June 11, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants