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

dist: don't push images #1365

Merged
merged 1 commit into from
Aug 16, 2021
Merged

dist: don't push images #1365

merged 1 commit into from
Aug 16, 2021

Conversation

mreiferson
Copy link
Member

The expected behavior is for dist.sh to produce a set of local images, to be able to separately push (which matches the behavior of what we do for .tar.gz files)

@mreiferson mreiferson added the bug label Aug 16, 2021
@ploxiln
Copy link
Member

ploxiln commented Aug 16, 2021

I think the problem is that you can't have both architectures of the same image tag tracked by the local docker daemon, so buildx has to push them as it makes them - you can't push both architectures later.

The docker buildx thing could be a separate even shorter script though.

(... I'm currently testing on a debian-11 host system with docker 20.10.5 from the base repos, buildx 0.6.1 plugin manually installed, and qemu 5.2.0 from the base repos with qemu-user-binfmt, and the multi-platform-at-once build fails strangely in some step in the middle, but one-at-a-time works, including for arm64. I'm guessing docker-for-mac works fine doing both concurrently.)

@ploxiln
Copy link
Member

ploxiln commented Aug 16, 2021

btw it looks like latest tag on docker hub only has amd64 (while v1.2.1 tag has both amd64 and arm64)

@mreiferson
Copy link
Member Author

Gonna try this docker/buildx#459 (comment)

@mreiferson
Copy link
Member Author

Updated, also fixed the latest tag in Docker registry.

docker buildx build --tag nsqio/nsq:latest . --platform linux/amd64,linux/arm64 --push
echo "Tagging nsqio/nsq:v$version as the latest release"
shas=$(docker manifest inspect nsqio/nsq:$version |\
grep digest | awk '{print $2}' | sed 's/[",]//g' | sed 's/^/nsqio\/nsq@/')
Copy link
Member

Choose a reason for hiding this comment

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

this looks like a job for jq - but if it works for you it's fine with me

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, of course. I wasn't quite sure what our policy for shell compatibility has been, although I suppose you already need quite a few dependencies (golang, docker).

@mreiferson mreiferson merged commit 12901cd into nsqio:master Aug 16, 2021
@mreiferson mreiferson deleted the dist-fix branch August 16, 2021 04:02
@danbf
Copy link
Contributor

danbf commented Aug 16, 2021

sorry for causing this noise, but #1365 (comment) is on the money here. locally you can't store the multi-arch builds :( so you have to buildx build and push in one step.

fi
docker buildx rm nsq-$rnd
Copy link
Contributor

@danbf danbf Aug 17, 2021

Choose a reason for hiding this comment

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

i think without the buildx rm the builders will pile up. not sure how much resources they use.

it might be possible to do an intelligent builder cleanup prior to setting up new builders using docker buildx ls and interpreting the results. for now on our systems if something happens and the builder gets orphan'd, i've just been removing it manually.

in our circle-ci setup on sso ( buzzfeed/sso#319 ) since we use ephemeral build machines we really don't have to care about the orphan builders.

Copy link
Member

Choose a reason for hiding this comment

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

I think one could also skip the create and use steps, and just buildx build for local builds, right? It would just cause build cache to accumulate until you docker system prune or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like having a namespace to inspect as an artifact of the build. I don't expect this script to be used in any automated context, so practically speaking it's fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

for local builds i think just doing an old school docker build is fine. it's really only for the multi-arch push that we need to do the create/use/build-and-push chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants