-
Notifications
You must be signed in to change notification settings - Fork 17
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
docker/install: Support version: master
#438
base: main
Are you sure you want to change the base?
Conversation
ea3d0b7
to
91a2ddd
Compare
5d3b32c
to
2941b11
Compare
src/docker/install.ts
Outdated
core.info(`Downloading Docker ${this.version} from ${this.channel}`); | ||
|
||
this._version = this.version; | ||
if (this.version == 'master') { |
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 think it would be better to rely on csv values so we can either choose archive or image format:
v24.0.9
similar totype=archive,version=v24.0.9
type=image,tag=master
type=image,tag=24.0.9
WDYT?
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.
But that would be smth to handle in setup-docker
action as well.
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.
Do we want to have another input variable for cli version alone?
Like:
version: type=image,tag=master # and optional repository=moby/moby-bin-someotherepo
cli-version: type=archive,version=v20.10.25 # if not specified, will default to tag from `version` an
or just sth like
version: |
component=engine,type=image,tag=master
component=cli,type=archive,version=v20.10.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.
Or, if we want to go in the direction of: crazy-max/ghaction-setup-docker#81
Then maybe we should have a separate image
input:
images: |
moby/moby-bin:master
dockereng/cli-bin:master
that would be mutually exclusive with version
?
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.
Do we want to have another input variable for cli version alone?
I think we should keep both under the same terminology instead of being separated. Don't think we want to allow custom registry or image repository imo.
Or, if we want to go in the direction of: crazy-max/ghaction-setup-docker#81
I think csv values like #438 (comment) are easier to maintain so we don't introduce a new input and keep backward compat with current version
input. I will update this proposal if we are ok with this. WDYT?
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.
My main concern here was the ability to install a different engine and cli version. I think it's fine to leave the image repository hardcoded, but it could be useful to be able to install different versions of CLI and Engine.
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.
but it could be useful to be able to install different versions of CLI and Engine.
Ok if there is this use case then what do you think of:
v24.0.9
>type=archive,version=v24.0.9
ortype=archive,engine_version=v24.0.9,cli_version=v24.0.9
if you want specific engine/cli version
Same for images:
type=image,tag=24.0.9
>type=image,engine_tag=24.0.9,cli_tag=24.0.9
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.
SGTM, although it's not an urgent use-case so we can handle that in a follow up.
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.
Updated to support the CSV syntax.
Example run: https://github.com/docker/moby-private/actions/runs/10773693004/job/29874033854?pr=9#step:2:336
7f4cb8d
to
9a7c8c8
Compare
Also opened a PR on the ghaction to make use of the new options: crazy-max/ghaction-setup-docker#106 |
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.
Along the change for source
can you add new cases in https://github.com/docker/actions-toolkit/blob/main/__tests__/docker/install.test.ts to download from image?
2c74c55
to
5a0fe00
Compare
Hum right, on macos runners it will use |
5a0fe00
to
97bef8d
Compare
Or just skip the daemon on darwin and only download the CLI? OTOH, this could be confusing behavior as it will not error out and won't be able to connect to the daemon.. |
97bef8d
to
20a30f2
Compare
Yeah provisioning for macos is a bit different. It downloads static bins from download.docker.com: https://github.com/docker/actions-toolkit/actions/runs/11289888751/job/31400662984#step:11:17 which only contains cli as expected. But then during lima provisioning it installs the engine using actions-toolkit/src/docker/assets.ts Line 224 in a59a5f8
|
Right, so we'd need to also handle the image download inside the lima provisioning script. I think it's easiest to just use undock there? The lima provisioning is already doing a lot of stuff, so the extra binary won't hurt there 😅 EDIT: Actually it might be easier to just mount the binaries to lima vm, let me try that. |
9662b9d
to
23c945d
Compare
Can you rebase to fix the QEMU issue related to #459 |
5234b17
to
bcb34bf
Compare
src/docker/assets.ts
Outdated
wget https://raw.githubusercontent.com/moby/moby/{{srcImageTag}}/contrib/init/systemd/docker.service \ | ||
https://raw.githubusercontent.com/moby/moby/v{{srcImageTag}}/contrib/init/systemd/docker.service \ | ||
-O /etc/systemd/system/docker.service || true | ||
wget https://raw.githubusercontent.com/moby/moby/{{srcImageTag}}/contrib/init/systemd/docker.socket \ | ||
https://raw.githubusercontent.com/moby/moby/v{{srcImageTag}}/contrib/init/systemd/docker.socket \ | ||
-O /etc/systemd/system/docker.socket || true | ||
mkdir -p /usr/local/bin | ||
cp /tool/* /usr/local/bin/ | ||
sed -i 's|^ExecStart=.*|ExecStart=/usr/local/bin/dockerd -H fd://|' /etc/systemd/system/docker.service | ||
sed -i 's|containerd.service||' /etc/systemd/system/docker.service | ||
if ! getent group docker; then | ||
groupadd --system docker | ||
fi | ||
systemctl daemon-reload | ||
if ! systemctl enable --now docker; then | ||
systemctl status docker.socket | ||
systemctl status docker.service | ||
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.
Maybe instead of doing this we could just run curl -fsSL https://get.docker.com | sh
and override binaries with ones from tooldir and restart daemon?
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 guess this could be the same behavior for both archive
and image
mode
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 was considering that, but that could be problematic in cases where there are packaging changes between the version installed by curl get.docker.com | sh
and the actual target version.
2ed7185
to
81d3249
Compare
Add support for installing Docker `master` packages from `moby/moby-bin` and `dockereng/cli-bin` images. This could also allow to install arbitrary version from these images but for now it's only used for `master`. Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
Use InstallSource instead Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
81d3249
to
668161b
Compare
Weird qemu failure:
Is this known to happen sometimes? 🤔 |
The previous failure on macos-12 was related to the long EDIT: Looks like it's still the case 🫠 |
41c8df2
to
0ab806d
Compare
Signed-off-by: Paweł Gronowski <[email protected]>
7ec9dc8
to
6bc0f4b
Compare
Signed-off-by: Paweł Gronowski <[email protected]>
6bc0f4b
to
8d62dc6
Compare
Add support for installing Docker
master
packages frommoby/moby-bin
anddockereng/cli-bin
images.This could also allow to install arbitrary version from these images but for now it's only used for
master
.Test run: https://github.com/docker/moby-private/actions/runs/10773693004/job/29874033854?pr=9#step:2:336