Skip to content

Add back push-manifests in Makefile #8254

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.

Sign up for GitHub

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

Merged
hjiawei merged 1 commit into from
Nov 22, 2023
Merged

Add back push-manifests in Makefile #8254

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib.Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,12 @@ push-image-arch-to-registry-%:
$(NOECHO) $(NOOP)\
)

# 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)))
Copy link
Member

@coutinhop coutinhop Nov 22, 2023

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:

calico/lib.Makefile

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

Copy link
Contributor Author

@hjiawei hjiawei Nov 22, 2023

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.

Copy link
Contributor Author

@hjiawei hjiawei Nov 22, 2023 •
edited
Loading

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.

Copy link
Member

@coutinhop coutinhop Nov 22, 2023

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

$(DOCKER) manifest push $(call unescapefs,$*):$(IMAGETAG)

# cd-common tags and pushes images with the branch name and git version. This target uses PUSH_IMAGES, BUILD_IMAGE,
# and BRANCH_NAME env variables to figure out what to tag and where to push it to.
cd-common: var-require-one-of-CONFIRM-DRYRUN var-require-all-BRANCH_NAME
Expand Down