🌱 Refactor docker-push* Makefile targets so users can control with ALL_DOCKER_BUILD which images are pushed

[AndiDog]

What this PR does / why we need it:

Previously, make docker-push-all would try to push the combined manifest and image of each component, while make docker-build-all would respect ALL_DOCKER_BUILD and only build the specified components. This change makes the Makefile target naming consistent (docker-push-<component> and docker-push-manifest-<component>, clusterctl not specially named anymore) and refactors so that ALL_DOCKER_BUILD defines which components are built and pushed.

Specifically, this allows a build of only production components using ALL_DOCKER_BUILD="core kubeadm-bootstrap kubeadm-control-plane".

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

n/a

[k8s-ci-robot]

Hi @AndiDog. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fabriziopandini]

/ok-to-test

cc @kubernetes-sigs/cluster-api-release-team

[furkatgofurov7]

Overall this change makes sense to me, however @AndiDog can you please rebase the PR?

[AndiDog]

Rebased by applying the patch also to the in-memory infrastructure provider Makefile targets

[fabriziopandini]

/lgtm
@killianmuldoon @sbueringer PTAL when you have time

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 6917729a11deff0e6df18e65c3da6d18eb2be75e

[fabriziopandini]

/unassign

[nawazkh]

/lgtm

[nawazkh]

/cc @sbueringer @killianmuldoon @CecileRobertMichon What do you all think about this?

[sbueringer]

Thx looks good to me, just two questions

Otherwise ready for merge

Sorry for the long delay

[sbueringer]

Suggested change

	ALL_DOCKER_BUILD = core kubeadm-bootstrap kubeadm-control-plane docker-infrastructure in-memory-infrastructure test-extension clusterctl
	ALL_DOCKER_BUILD ?= core kubeadm-bootstrap kubeadm-control-plane docker-infrastructure in-memory-infrastructure test-extension clusterctl

Q: Do we have to add this to make it overwritable via env var?

I assume not, just no idea why we usually use ?= for stuff like this :)

[Jont828]

I believe you do. Using ?= allows the env var to take precedence if it's set whereas = just ignores the env var.

[sbueringer]

Hm, this would mean the PR wouldn't work in its current state, right?

[AndiDog]

I made this configurable since fork builds (as my company's) may not require building all components. For example, only the controllers may be required but not the test or CLI images.

[sbueringer]

I got this part. My question is, is it currently possible to overwrite this via an env var without changing the above to ?=?

(basically a newbie Makefile question :))

[sbueringer]

➜  export ALL_DOCKER_BUILD=core
➜ make docker-build
...
➜ /Library/Developer/CommandLineTools/usr/bin/make ARCH=arm64 docker-build-core docker-build-kubeadm-bootstrap docker-build-kubeadm-control-plane docker-build-docker-infrastructure docker-build-in-memory-infrastructure docker-build-test-extension docker-build-clusterctl

[AndiDog]

Ah sorry, I thought this was part of my changes already but it's a GitHub suggested change 🙈.

We definitely use this change with overridden ALL_DOCKER_BUILD already, and make ALL_DOCKER_BUILD="core" ARCH=boom REGISTRY=myreg docker-push on macOS works fine to build only the core image. I found it's because make will override variables if you set them on the command line. Therefore, ALL_DOCKER_BUILD="core" ARCH=boom REGISTRY=myreg make docker-push, i.e. using real environment variables only, doesn't work. I will quickly test again and switch multiple places to ?=. Good find, thx!!

[sbueringer]

Ah thank you very much, that is good to know :)

[AndiDog]

Unless you don't want to make environment vs. command line parameters the same. Not sure if that is desired?!

[sbueringer]

I think it would be good if both works

[CecileRobertMichon]

/cc @Jont828

[Jont828]

/lgtm

[Jont828]

@CecileRobertMichon I think this should work as is for any of our use cases since it would use the default values unless specified.

[sbueringer]

Thank you very much!!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: c2fb9a47e81fcf984db3b07617ec6e657f9af980

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

/area devtools

[sbueringer]

I don't expect any problems, but let's verify that everything works as expected by checking the logs of https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/post-cluster-api-push-images/1691699431717474304
(should finish in ~15-20 min)

[sbueringer]

Looks all good to me, e.g.

➜ docker buildx imagetools inspect gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217
Name:      gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217
MediaType: application/vnd.docker.distribution.manifest.list.v2+json
Digest:    sha256:01c215327091a4aeb0d0c34a60b4dbcdc4fc53e1aa03e5507000ae9dfc4d1b6d

Manifests:
  Name:      gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:420e6bd0557990e026abebda0f18aeec0a65c878fe7c48b19e726b49caf907ce
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/amd64

  Name:      gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:f6836cff88a704d87eb3c0164c4b8e5d0fda54a02764ed7eff3071d9303dccce
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/arm

  Name:      gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:34c7ae653098373a65d53b2756ea6249288484342c43a64916d54a25fe906e0b
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/arm64

  Name:      gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:9afbc9f3a88d037099f6e72ec9205d0abac42d8c96409684ad7f1a0d787c1795
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/ppc64le

  Name:      gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:a09ace82c8d92ee5768820d436c1e84175f55bd3a7b8e031fb5863c8ccc93a41
  MediaType: application/vnd.docker.distribution.manifest.v2+json
  Platform:  linux/s390x