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

ci: Add bin-image workflow #4752

Merged
merged 3 commits into from
Jan 12, 2024
Merged

ci: Add bin-image workflow #4752

merged 3 commits into from
Jan 12, 2024

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Jan 4, 2024

- What I did
Build and push an image containing a static CLI binary for master branch and every release branch and tag.

This is a slightly adjusted copy of the bin-image workflow from moby/moby (by @crazy-max).

- How I did it
See individual commits.

- How to verify it
Tested against my fork:

v29.0.0 tag: https://github.com/vvoland/cli/actions/runs/7410112486/job/20161818586

$ printf 'FROM alpine
   COPY --from=pawelgronowski465/cli-bin:29.0.0 /docker /docker
   RUN /docker version' | docker build -
...
0.119 Client:
0.119  Version:           29.0.0
0.119  API version:       1.44
0.119  Go version:        go1.21.5
0.119  Git commit:        d1b8d3ce50
0.119  Built:             Thu Jan  4 13:26:07 2024
0.119  OS/Arch:           linux/arm64
0.119  Context:           default

29.0 branch: https://github.com/vvoland/cli/actions/runs/7409929402/job/20161236975

$ printf 'FROM alpine
   COPY --from=pawelgronowski465/cli-bin:29.0 /docker /docker
   RUN /docker version' | docker build -
...
0.162 Client:
0.162  Version:           29.0
0.162  API version:       1.44
0.162  Go version:        go1.21.5
0.162  Git commit:        a1f9f0d681
0.162  Built:             Thu Jan  4 13:10:11 2024
0.162  OS/Arch:           linux/arm64
0.162  Context:           default
...

ci-bin-image-test branch: https://github.com/vvoland/cli/actions/runs/7410075016/job/20161631291

$ rintf 'FROM alpine
   COPY --from=pawelgronowski465/cli-bin:ci-bin-image-test /docker /docker
   RUN /docker version' | docker build -
...
0.153 Client:
0.153  Version:           ci-bin-image-test
0.153  API version:       1.44
0.153  Go version:        go1.21.5
0.153  Git commit:        d1b8d3ce50
0.153  Built:             Thu Jan  4 13:22:07 2024
0.153  OS/Arch:           linux/arm64
0.153  Context:           default
...

- Description for the changelog

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

Merging #4752 (3d16840) into master (859154b) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4752   +/-   ##
=======================================
  Coverage   59.61%   59.61%           
=======================================
  Files         287      287           
  Lines       24784    24784           
=======================================
  Hits        14775    14775           
  Misses       9121     9121           
  Partials      888      888           

type=ref,event=pr
type=sha
-
name: Login to DockerHub
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to skip login on PR:

Suggested change
name: Login to DockerHub
name: Login to DockerHub
if: github.event_name != 'pull_request'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will be removed once the PR is undrafted, this is all a part of a separate debug commit that just enables the worflow to run for this PR: 23c5809

`scripts/make/binary` produces `docker` file that is a symlink to a
`docker-<platform>` file.
Make the `binary` Dockerfile target produce an image that only contains
the `docker` binary and not the symlink.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Transform `VERSION` variable if it contains a git ref.
This is the same as moby does (with "<<<" bashism removed).

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Build and push an image containing a static CLI binary for master branch
and every release branch and tag.

This is a slightly adjusted copy of the bin-image workflow from
docker/buildx (by @crazy-max).

Co-authored-by: CrazyMax <crazy-max@users.noreply.github.com>

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the ci-bin-image branch 2 times, most recently from 3bb23b3 to 3d16840 Compare January 12, 2024 12:07
COPY --from=build /out .
COPY --from=build /out/docker /docker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this COPY had to be changed; did we add more things in /out ? Where possible, I like “copy all the things” so that the part copying things doesn’t need to know what to look for

(basically "/out" being the "contract" / API (for better words)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scripts/build/binary produces two files, first is docker-<platform>-<arch> which is an actual binary, and the second is a docker which is a symlink to the former.

So without this change the image content would be:

/docker-<platform>-<arch>        # the actual binary
/docker                          # symlink to the above

The COPY copies the /docker which dereferences into the actual binary and puts that directly as /docker in the final image. So the image only contains the docker binary directly.

It's not strictly needed, because it should work with COPY --from=dockereng/cli /docker /docker anyway, but I'm not sure if we really need that symlink in the final image.

Alternatively I could look into changing the scripts/build/binary so that it doesn't output the symlink, but that would probably require changing other things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, forgot about the symlink; yeah. not a blocker, just something "nice if we can improve"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL, and of course I didn't think of docker-ce-packaging using this output, and now failng;

dest=$PWD/build/mac; cd /home/ubuntu/workspace/release-packaging_ce-nightly/packaging/src/github.com/docker/cli/build && for platform in *; do \
	arch=$(echo $platform | cut -d_ -f2); \
	mkdir -p $dest/$arch/docker; \
	cp $platform/docker-darwin-* $dest/$arch/docker/docker && \
	tar -C $dest/$arch -c -z -f $dest/$arch/docker-25.0.0-rc.2.tgz docker; \
done
cp: cannot stat 'darwin_amd64/docker-darwin-*': No such file or directory
cp: cannot stat 'darwin_arm64/docker-darwin-*': No such file or directory

@vvoland vvoland marked this pull request as ready for review January 12, 2024 14:02
@vvoland
Copy link
Collaborator Author

vvoland commented Jan 12, 2024

FYI, the secrets are filled and the PR is ready for review.

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah added this to the 25.0.0 milestone Jan 12, 2024
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; let's get this one in and check if it all works 👍

@thaJeztah thaJeztah merged commit d469be2 into docker:master Jan 12, 2024
77 checks passed
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