🌱 Minor CAPD Makefile cleanup #4418
Conversation
k8s-ci-robot
added
kind/cleanup
|
cc @vincepri |
k8s-ci-robot
added
the
size/S
| @@ -94,11 +94,11 @@ test-verbose: ## Run tests with verbose settings. | |||
| manager: ## Build manager binary | |||
| go build -o $(BIN_DIR)/manager sigs.k8s.io/cluster-api/test/infrastructure/docker | |||
|
|
|||
| $(CONTROLLER_GEN): $(TOOLS_DIR)/go.mod # Build controller-gen from tools folder. | |||
| cd $(TOOLS_DIR); go build -tags=tools -o $(BIN_DIR)/controller-gen sigs.k8s.io/controller-tools/cmd/controller-gen | |||
| $(CONTROLLER_GEN): | |||
There was a problem hiding this comment.
I'm getting more and more confused about Makefiles work :), but do you need those targets at all?
Can you rebase onto the current master? I'm also using gotestsum here and I didn't have to specify a target like those.
There was a problem hiding this comment.
In a scenario where a user wants to run make generate inside CAPD, we need to build them if I am not missing anything.
There was a problem hiding this comment.
I dropped those targets locally and it still seems to work:
-> cd ./test/infrastructure/docker
-> make generate-go
~/code/src/sigs.k8s.io/cluster-api/hack/tools/bin/controller-gen \
...Same for make test-junit:
...
{"Time":"2021-03-31T20:22:24.872626044+02:00","Action":"skip","Package":"sigs.k8s.io/cluster-api/test/infrastructure/docker/third_party/forked/loadbalancer","Elapsed":0}
~/code/src/sigs.k8s.io/cluster-api/hack/tools/bin/gotestsum --junitfile ../../../_artifacts/junit.infra_docker.xml --raw-command cat ../../../_artifacts/junit.infra_docker.stdout
✓ cloudinit (cached)
✓ api/v1alpha4 (cached)
...I suspect it just resolves the path and builds the targets already defined in the top-level file.
But it's very possible that I'm missing something
(I think the same would apply to $(KUSTOMIZE))
There was a problem hiding this comment.
If those binaries are not already in hack/tools/bin before, not working for me.
There was a problem hiding this comment.
I didn't think of that. Same for me, I just already had the binaries before I tested it.
There was a problem hiding this comment.
Can you maybe also add a target for GOTESTSUM? I missed that use case with my PR
|
Thank you! /lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
A minor cleanup to use the main Makefile targets for CAPD controller-gen and conversion-gen.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Related to #4407
/kind cleanup