🌱 Add versioned tool installation via go-install-tool helper in Kubebuilder Makefile such as we provide for end users#4986
Conversation
- Introduced `LOCALBIN` to define local binary installation directory (`./bin`). - Added a reusable `go-install-tool` Make function to install Go tools with version pinning. - Refactored `golangci-lint` and `go-apidiff` targets to use `go-install-tool`. - Defined tool binary paths (`GO_APIDIFF`, `GOLANGCI_LINT`) and their corresponding version variables.
|
Hi @afritzler. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a standardized tool installation system for Go-based development tools using Make functions. The main goal is to enable versioned tool installation with proper dependency management in the local ./bin directory.
- Adds a reusable
go-install-toolMake function for versioned Go tool installation - Refactors existing tool installation targets (
golangci-lint,go-apidiff) to use the new system - Introduces proper version pinning and local binary management
| # $2 - package url which can be installed | ||
| # $3 - specific version of package | ||
| define go-install-tool | ||
| @[ -f "$(1)-$(3)" ] || { \ |
There was a problem hiding this comment.
The condition checks for a versioned binary file "$(1)-$(3)" but this file may not exist on first installation. The logic should check if the symlink $(1) points to the correct versioned binary instead of relying on the existence of the versioned file.
| @[ -f "$(1)-$(3)" ] || { \ | |
| @[ -f "$(1)-$(3)" ] && [ "$$(readlink -- "$(1)" 2>/dev/null)" = "$(1)-$(3)" ] || { \ |
There was a problem hiding this comment.
We might do the AI suggestion for the Makefile end users scaffolded too.
So, I will let it be a follow-up.
Where can we change all at once
Could you work on the follow-up?
We will need to change:
kubebuilder/pkg/plugins/golang/v4/scaffolds/internal/templates/makefile.go
Lines 303 to 317 in 98bc8f4
And run make install AND make generate
| GOBIN=$(LOCALBIN) go install $${package} ;\ | ||
| mv $(1) $(1)-$(3) ;\ | ||
| } ;\ | ||
| ln -sf $(1)-$(3) $(1) |
There was a problem hiding this comment.
The symlink creation uses a relative path which may not work correctly. The target should use an absolute path or basename to ensure the symlink points to the correct file: ln -sf $(notdir $(1)-$(3)) $(1)
| ln -sf $(1)-$(3) $(1) | |
| ln -sf $$(realpath $(1)-$(3)) $(1) |
| set -e; \ | ||
| package=$(2)@$(3) ;\ | ||
| echo "Downloading $${package}" ;\ | ||
| rm -f $(1) || true ;\ |
There was a problem hiding this comment.
The || true is redundant since rm -f never fails. Consider removing it for cleaner code: rm -f $(1) ;\
| rm -f $(1) || true ;\ | |
| rm -f $(1) ;\ |
There was a problem hiding this comment.
Hi @afritzler
Thank you for your contribution 🥇
Because this change is in the Kubebebuilder makefile, it does not impact the end users.
Therefore, the right emoji is 🌱
Why does that matter?
We use it to do the release notes. So, I will update the PR title. I hope that you do not mind.
go-install-tool helpergo-install-tool helper in Kubebuilder Makefile such as we provide for end users
|
/approved @afritzler could we please do the changes in a follow up: #4986 (comment) Let's apply the AI suggestions for both here and end-users scaffolds. |
|
@camilamacedo86 thanks for your feedback! I will incorporate the suggested improvements. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afritzler, camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Suggested Changes
LOCALBINto define local binary installation directory (./bin).go-install-toolMake function to install Go tools with version pinning.golangci-lintandgo-apidifftargets to usego-install-tool.GO_APIDIFF,GOLANGCI_LINT) and their corresponding version variables.