-
Notifications
You must be signed in to change notification settings - Fork 301
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
Use Docker CLI packages, update Docker Compose, and update centos to 8.x #1351
Conversation
Signed-off-by: Remco de Man <[email protected]>
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!
I left a few small comments, but no major blockers from my perspective.
packaging/docker/alpine-linux/buildkite-agent | ||
packaging/docker/ubuntu-linux/buildkite-agent | ||
packaging/docker/*/buildkite-agent | ||
packaging/docker/*/hooks/ |
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.
👌
@@ -1,24 +1,28 @@ | |||
FROM centos:7.7.1908 | |||
FROM centos:8.3.2011 |
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.
I'm not very familiar with centos, but is jumping from 7.x to 8.x significant?
Assuming 8.x is stable then I don't see a problem with it (we recently bumped alpine and ubuntu by major versions as well), but it might be worth calling out in the PR description to raise the visibility of the change.
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.
CentOS 8.x is their current stable release: I see no reason not to bump it. I found no issues with it either.
I think this is probably fine? I know of many folks who mount the host docker socket and/or use docker-in-docker via a sidecar, but I haven't come across folk running the docker daemon within the agent container. I might chat to folks internally to double check my assumptions here. Also worth noting, but probably not a problem: this seems to upgrade the docker cli from 19.03 to 20.10. |
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.
Lovely!
We're still discussing the docker vs docker-cli question internally, but this PR is a nett win regardless of that discussion so I'll merge.
If we decide the docker daemon should stay, I'll restore it in a follow up PR prior to the next release.
This basically replaces #1214 as it seems to be abandoned and we would really like to see the Docker Compose version upgraded.
This PR upgrades de
tini
anddocker-compose
binaries to their newest versions on Ubuntu & CentOS. It also replaces the deprecateddocker.io
package with the installation method described on https://docs.docker.com/engine/install/ubuntu/ for Ubuntu.The big difference with this PR in comparison to #1214 is the bare inclusion of Docker's CLI packages. No longer is the whole Docker engine included, as I suspect most people will just bind mount the socket inside the Docker container. Please let me know if this is undesirable somehow, then I will add the engine back.
Centos is also bumped from 7.x to 8.x, the current stable release.