-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add back push-manifests in Makefile #8254
Conversation
I don't have enough permission to do a real manifest push but I think the following dryrun output is what we need. For example, from calicoctl folder:
|
# push multi-arch manifest where supported. | ||
push-manifests: var-require-all-IMAGETAG $(addprefix sub-manifest-,$(call escapefs,$(PUSH_MANIFEST_IMAGES))) | ||
sub-manifest-%: | ||
$(DOCKER) manifest create $(call unescapefs,$*):$(IMAGETAG) $(addprefix --amend ,$(addprefix $(call unescapefs,$*):$(IMAGETAG)-,$(VALIDARCHES))) |
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.
any specific reason not to restore MANIFEST_TOOL
and MANIFEST_TOOL_CMD
exactly as it is in https://github.com/projectcalico/calico/pull/8103/files#diff-abfd44a7d28b43da680f905985a06fe893b7e4aa8920d87fee8070b261645b55L915?
In particular, I believe the -v $(DOCKER_CONFIG):/root/.docker/config.json
arg will be necessary for credentials when running on semaphore... I had to do something similar for windows manifests:
Lines 1319 to 1345 in 004a6e3
# When running on semaphore, just copy the docker config, otherwise run | |
# 'docker-credential-gcr configure-docker' as well. | |
ifdef SEMAPHORE | |
DOCKER_CREDENTIAL_CMD = cp /root/.docker/config.json_host /root/.docker/config.json | |
else | |
DOCKER_CREDENTIAL_CMD = cp /root/.docker/config.json_host /root/.docker/config.json && \ | |
docker-credential-gcr configure-docker | |
endif | |
# This needs the $(WINDOWS_DIST)/bin/docker-credential-gcr binary in $PATH and | |
# also the local ~/.config/gcloud dir to be able to push to gcr.io. It mounts | |
# $(DOCKER_CONFIG) and copies it so that it can be written to on the container, | |
# but not have any effect on the host config. | |
CRANE_BINDMOUNT_CMD := \ | |
docker run --rm \ | |
--net=host \ | |
--init \ | |
--entrypoint /bin/sh \ | |
-e LOCAL_USER_ID=$(LOCAL_USER_ID) \ | |
-v $(CURDIR):/go/src/$(PACKAGE_NAME):rw \ | |
-v $(DOCKER_CONFIG):/root/.docker/config.json_host:ro \ | |
-e PATH=$${PATH}:/go/src/$(PACKAGE_NAME)/$(WINDOWS_DIST)/bin \ | |
-v $(HOME)/.config/gcloud:/root/.config/gcloud \ | |
-w /go/src/$(PACKAGE_NAME) \ | |
$(CALICO_BUILD) -c $(double_quote)$(DOCKER_CREDENTIAL_CMD) && crane | |
DOCKER_MANIFEST_CMD := docker manifest |
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.
The manifest tool is removed from go-build v0.90. docker manifest command (even though still experimental) was merged into docker cli back in 2017. After that, projects from kubernetes, aws, ibm switched to that. I think it would make more sense for us to use the official utility 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.
For semaphore build, we do docker login first. docker manifest command will use the same credential once we login to a registry.
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.
Oh I see! Yes, just noticed that the manifest cmd for the windows images is just docker manifest
Reimplement `push-manifests` in `lib.Makefile` using `docker manifest` command [1]. It was accidentally removed in [2]. [1] https://docs.docker.com/engine/reference/commandline/manifest/ [2] https://github.com/projectcalico/calico/pull/8103/files#diff-abfd44a7d28b43da680f905985a06fe893b7e4aa8920d87fee8070b261645b55L915
60af20f
to
1123744
Compare
Reimplement `push-manifests` in `Makefile.common` using `docker manifest` command [1]. It was removed in [2]. This change is ported from [3]. [1] https://docs.docker.com/engine/reference/commandline/manifest/ [2] https://github.com/projectcalico/go-build/pull/490/files#diff-f190d29e2cad4f32539c8b73d8af07c77f0aac63b5c6553c06b117c08a70b494L876 [3] projectcalico/calico#8254
Reimplement `push-manifests` in `Makefile.common` using `docker manifest` command [1]. It was removed in [2]. This change is ported from [3]. [1] https://docs.docker.com/engine/reference/commandline/manifest/ [2] https://github.com/projectcalico/go-build/pull/490/files#diff-f190d29e2cad4f32539c8b73d8af07c77f0aac63b5c6553c06b117c08a70b494L876 [3] projectcalico/calico#8254
Description
Reimplement
push-manifests
inlib.Makefile
usingdocker manifest
command [1]. It was accidentally removed in [2].[1] https://docs.docker.com/engine/reference/commandline/manifest/
[2] https://github.com/projectcalico/calico/pull/8103/files#diff-abfd44a7d28b43da680f905985a06fe893b7e4aa8920d87fee8070b261645b55L915
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*
label.docs-pr-required
: This change requires a change to the documentation that has not been completed yet.docs-completed
: This change has all necessary documentation completed.docs-not-required
: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*
label.release-note-required
: This PR has user-facing changes. Most PRs should have this label.release-note-not-required
: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate
: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr
: This PR is related to install and requires a corresponding change to the operator.