🌱 Add make commands for CAPI operator #4126

wfernandes · 2021-01-26T23:08:55Z

What this PR does / why we need it:
This PR adds a bunch of make commands for the CAPI operator. It is built on top of the PR #4117.
This PR should be merged after #4117.

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

wfernandes · 2021-01-26T23:09:25Z

/hold
Putting this on hold because this PR should be merged in after #4117.

wfernandes · 2021-01-26T23:09:55Z

/area api
/area operator

k8s-ci-robot · 2021-01-26T23:09:56Z

@wfernandes: The label(s) area/operator cannot be applied, because the repository doesn't have them

In response to this:

/area api
/area operator

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.

wfernandes · 2021-01-28T23:39:23Z

/retest

vincepri · 2021-01-29T16:57:13Z

What do you think about having a separate Makefile for the operator in the relative folder and use $(MAKE) -c /path/to/operator to run commands in those?

We should probably do the same for the bootstrap, control plane providers, etc

wfernandes · 2021-01-29T18:12:45Z

Sure. I was just being consistent with what we have now.

I don't mind breaking it up into several Makefiles but can we do that as part of a separate issue. I'm happy to create the issue describing Makefiles for each CAPI component that makes sense.
WDYT?

wfernandes · 2021-01-29T21:42:35Z

/retest

vincepri · 2021-02-01T18:28:10Z

/hold

k8s-ci-robot · 2021-03-04T01:47:11Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

fabriziopandini · 2021-03-10T13:47:29Z

Makefile

@@ -237,7 +230,7 @@ generate-go: $(GOBINDATA) ## Runs Go related generate targets
 	$(MAKE) generate-go-core
 	$(MAKE) generate-go-kubeadm-bootstrap
 	$(MAKE) generate-go-kubeadm-control-plane
-	$(MAKE) generate-go-operator
+	$(MAKE) -C $(OPERATOR_DIR) generate-go-operator


I think we should not call the operator targets by default.
Let's call them explicitly, like we are doing for CAPD

I thought the operator is a bit different, don't we plan to ship it the same way as kubeadm stuff?

This makes sense IMO, and it will help identify early issues during manifest generation, when generated resources will overlap between CAPI and CAPIO. CI should handle this part and prevent from PRs being merged if this task fails.

fabriziopandini · 2021-03-10T13:48:36Z

Makefile

-	DOCKER_BUILDKIT=1 docker build --build-arg goproxy=$(GOPROXY) --build-arg ARCH=$(ARCH) --build-arg package=./exp/operator --build-arg ldflags="$(LDFLAGS)" . -t $(OPERATOR_CONTROLLER_IMG)-$(ARCH):$(TAG)
-	$(MAKE) set-manifest-image MANIFEST_IMG=$(OPERATOR_CONTROLLER_IMG)-$(ARCH) MANIFEST_TAG=$(TAG) TARGET_RESOURCE="./exp/operator/config/default/manager_image_patch.yaml"
-	$(MAKE) set-manifest-pull-policy TARGET_RESOURCE="./exp/operator/config/default/manager_pull_policy.yaml"
+	$(MAKE) -C $(OPERATOR_DIR) docker-build-operator


this target should go away from the main docker file

fabriziopandini · 2021-03-10T13:48:45Z

Makefile

-	@for arch in $(ALL_ARCH); do docker manifest annotate --arch $${arch} ${OPERATOR_CONTROLLER_IMG}:${TAG} ${OPERATOR_CONTROLLER_IMG}-$${arch}:${TAG}; done
-	docker manifest push --purge $(OPERATOR_CONTROLLER_IMG):$(TAG)
-	$(MAKE) set-manifest-image MANIFEST_IMG=$(OPERATOR_CONTROLLER_IMG) MANIFEST_TAG=$(TAG) TARGET_RESOURCE="./exp/operator/config/default/manager_image_patch.yaml"
+	$(MAKE) -C "./$(OPERATOR_DIR)/" docker-push-operator-manifest


this target should go away from the main docker file

fabriziopandini · 2021-03-10T13:49:20Z

Makefile

@@ -315,7 +302,7 @@ generate-manifests: ## Generate manifests e.g. CRD, RBAC etc.
 	$(MAKE) generate-core-manifests
 	$(MAKE) generate-kubeadm-bootstrap-manifests
 	$(MAKE) generate-kubeadm-control-plane-manifests
-	$(MAKE) generate-operator-manifests
+	$(MAKE) -C $(OPERATOR_DIR) generate-operator-manifests


fabriziopandini · 2021-03-10T13:50:15Z

Makefile

@@ -546,7 +518,7 @@ release-manifests: $(RELEASE_DIR) $(KUSTOMIZE) ## Builds the manifests to publis
 	# Build control-plane-components.
 	$(KUSTOMIZE) build controlplane/kubeadm/config/default > $(RELEASE_DIR)/control-plane-components.yaml
 	# Build operator components.
-	$(KUSTOMIZE) build exp/operator/config/default > $(RELEASE_DIR)/operator-components.yaml
+	$(KUSTOMIZE) build $(OPERATOR_DIR)/config/default > $(RELEASE_DIR)/operator-components.yaml


This should go in the operator make file

fabriziopandini · 2021-03-10T13:50:23Z

Makefile

 	$(MAKE) set-manifest-pull-policy PULL_POLICY=IfNotPresent TARGET_RESOURCE="./config/default/manager_pull_policy.yaml"
 	$(MAKE) set-manifest-pull-policy PULL_POLICY=IfNotPresent TARGET_RESOURCE="./bootstrap/kubeadm/config/default/manager_pull_policy.yaml"
 	$(MAKE) set-manifest-pull-policy PULL_POLICY=IfNotPresent TARGET_RESOURCE="./controlplane/kubeadm/config/default/manager_pull_policy.yaml"
-	$(MAKE) set-manifest-pull-policy PULL_POLICY=IfNotPresent TARGET_RESOURCE="./exp/operator/config/default/manager_pull_policy.yaml"
+	$(MAKE) set-manifest-pull-policy PULL_POLICY=IfNotPresent TARGET_RESOURCE="./$(OPERATOR_DIR)/config/default/manager_pull_policy.yaml"


This should go in the operator make file

fabriziopandini · 2021-03-10T13:50:28Z

Makefile

@@ -531,11 +503,11 @@ manifest-modification: # Set the manifest images to the staging/production bucke
 		TARGET_RESOURCE="./controlplane/kubeadm/config/default/manager_image_patch.yaml"
 	$(MAKE) set-manifest-image \
 		MANIFEST_IMG=$(PROD_REGISTRY)/$(OPERATOR_IMAGE_NAME) MANIFEST_TAG=$(RELEASE_TAG) \
-		TARGET_RESOURCE="./exp/operator/config/default/manager_image_patch.yaml"
+		TARGET_RESOURCE="./$(OPERATOR_DIR)/config/default/manager_image_patch.yaml"


This should go in the operator make file

My idea was to reuse targets like set-manifest-image instead of duplicating them across Makefiles. Another approach might be a Makefile that has all common targets and this Makefile is then included in all others.

Danil-Grigorev · 2021-03-10T15:20:59Z

Makefile

@@ -237,7 +230,7 @@ generate-go: $(GOBINDATA) ## Runs Go related generate targets
 	$(MAKE) generate-go-core
 	$(MAKE) generate-go-kubeadm-bootstrap
 	$(MAKE) generate-go-kubeadm-control-plane
-	$(MAKE) generate-go-operator
+	$(MAKE) -C $(OPERATOR_DIR) generate-go-operator


This makes sense IMO, and it will help identify early issues during manifest generation, when generated resources will overlap between CAPI and CAPIO. CI should handle this part and prevent from PRs being merged if this task fails.

Danil-Grigorev · 2021-03-10T15:26:57Z

Makefile

@@ -405,6 +415,7 @@ docker-push-all: $(addprefix docker-push-,$(ALL_ARCH))
 	$(MAKE) docker-push-core-manifest
 	$(MAKE) docker-push-kubeadm-bootstrap-manifest
 	$(MAKE) docker-push-kubeadm-control-plane-manifest
+	$(MAKE) docker-push-operator-manifest


I would argue that this task should be independent from core Makefile. Until the operator is experimental, default push command should accidentally create new images in release.

alexander-demicev · 2021-03-10T21:54:55Z

A small recap:

Added a separate Makefile for the operator(similar to CAPD) and separated the PR into meaningful commits.
Added context to Reconcile() arguments and removed zap logger from test. Without these 2 changes, the project fails to build.
Couple of other minor changes.

MarcelMue

I would like to go forward with this for now. We will have an easier time iterating then.

fabriziopandini · 2021-03-11T18:04:44Z

+1 to get this moving.
Please squash commits

Rebased and fix version pkg path Remove zap logger Remove .gitignore, Dockerfile and modules files Add context to reconciler arguments Add test tasks for operator Add binary building tasks for operator Add linting tasks for operator Add generate tasks for operator Add Docker tasks for operator Add release command for operator Add cleanup tasks for operator Make operator more consistent with other projects

MarcelMue · 2021-03-12T20:08:18Z

Good to go from my side

/LGTM

CecileRobertMichon · 2021-03-12T20:24:13Z

/approve

k8s-ci-robot · 2021-03-12T20:24:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

~~exp/operator/OWNERS~~ [CecileRobertMichon]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2021

k8s-ci-robot requested review from CecileRobertMichon and JoelSpeed January 26, 2021 23:09

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2021

k8s-ci-robot added the area/api Issues or PRs related to the APIs label Jan 26, 2021

wfernandes mentioned this pull request Jan 27, 2021

✨ Initial scaffolding for operator #4117

Merged

wfernandes force-pushed the capi-operator-make branch 2 times, most recently from f065618 to c0c586f Compare January 28, 2021 04:45

wfernandes mentioned this pull request Jan 29, 2021

🌱 Enable Webhooks and CertManager for CAPI operator #4132

Closed

wfernandes force-pushed the capi-operator-make branch from c0c586f to bb76b00 Compare January 29, 2021 16:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2021

wfernandes marked this pull request as draft February 9, 2021 22:50

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2021

wfernandes changed the base branch from master to operator-0.4 February 23, 2021 18:21

alexander-demicev force-pushed the capi-operator-make branch from 8c83fac to 7c6fb49 Compare March 1, 2021 11:52

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2021

wfernandes marked this pull request as ready for review March 4, 2021 01:46

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2021

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 4, 2021

This was referenced Mar 8, 2021

CAPI: Add operator branch to main jobs kubernetes/test-infra#21182

Merged

Make operator consistent with other CAPI parts #4246

Closed

alexander-demicev force-pushed the capi-operator-make branch from 4f9d781 to 4dfe263 Compare March 10, 2021 13:32

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 10, 2021

alexander-demicev force-pushed the capi-operator-make branch from 4dfe263 to 391bd29 Compare March 10, 2021 13:35

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 10, 2021

fabriziopandini reviewed Mar 10, 2021

View reviewed changes

alexander-demicev force-pushed the capi-operator-make branch 2 times, most recently from a7cd7eb to 9972a59 Compare March 10, 2021 14:04

Danil-Grigorev reviewed Mar 10, 2021

View reviewed changes

alexander-demicev force-pushed the capi-operator-make branch from 9972a59 to 3ace910 Compare March 10, 2021 21:47

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 10, 2021

alexander-demicev force-pushed the capi-operator-make branch from 3ace910 to a5367e2 Compare March 10, 2021 21:51

MarcelMue approved these changes Mar 11, 2021

View reviewed changes

alexander-demicev force-pushed the capi-operator-make branch from a5367e2 to 4724d32 Compare March 12, 2021 11:53

alexander-demicev force-pushed the capi-operator-make branch from 4724d32 to f481922 Compare March 12, 2021 15:57

k8s-ci-robot assigned MarcelMue Mar 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2021

k8s-ci-robot merged commit 337ea36 into kubernetes-sigs:operator-0.4 Mar 12, 2021

alexander-demicev mentioned this pull request Mar 15, 2021

Create make commands for the operator #4122

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add make commands for CAPI operator #4126

🌱 Add make commands for CAPI operator #4126

wfernandes commented Jan 26, 2021

wfernandes commented Jan 26, 2021

wfernandes commented Jan 26, 2021

k8s-ci-robot commented Jan 26, 2021

wfernandes commented Jan 28, 2021

vincepri commented Jan 29, 2021

wfernandes commented Jan 29, 2021

wfernandes commented Jan 29, 2021

vincepri commented Feb 1, 2021

k8s-ci-robot commented Mar 4, 2021

fabriziopandini Mar 10, 2021

alexander-demicev Mar 10, 2021

Danil-Grigorev Mar 10, 2021

fabriziopandini Mar 10, 2021

fabriziopandini Mar 10, 2021

fabriziopandini Mar 10, 2021

fabriziopandini Mar 10, 2021

fabriziopandini Mar 10, 2021

fabriziopandini Mar 10, 2021

alexander-demicev Mar 10, 2021

Danil-Grigorev Mar 10, 2021

Danil-Grigorev Mar 10, 2021

alexander-demicev commented Mar 10, 2021

MarcelMue left a comment

fabriziopandini commented Mar 11, 2021

MarcelMue commented Mar 12, 2021

CecileRobertMichon commented Mar 12, 2021

k8s-ci-robot commented Mar 12, 2021

🌱 Add make commands for CAPI operator #4126

🌱 Add make commands for CAPI operator #4126

Conversation

wfernandes commented Jan 26, 2021

wfernandes commented Jan 26, 2021

wfernandes commented Jan 26, 2021

k8s-ci-robot commented Jan 26, 2021

wfernandes commented Jan 28, 2021

vincepri commented Jan 29, 2021

wfernandes commented Jan 29, 2021

wfernandes commented Jan 29, 2021

vincepri commented Feb 1, 2021

k8s-ci-robot commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-demicev commented Mar 10, 2021

MarcelMue left a comment

Choose a reason for hiding this comment

fabriziopandini commented Mar 11, 2021

MarcelMue commented Mar 12, 2021

CecileRobertMichon commented Mar 12, 2021

k8s-ci-robot commented Mar 12, 2021