chore: Improve performance of make verify#1382
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request improves the performance of the make verify target by introducing conditional tool installation. Currently, make verify reinstalls all tools every time, which takes approximately 40 seconds. With this change, tools that are already present will be skipped, significantly reducing verification time.
Changes:
- Added
SKIP_INSTALLEDenvironment variable support tohack/toolchain.shto enable conditional tool installation - Refactored tool installation to use helper functions
should-skipandgo-installthat check for tool presence before installing - Modified the
verifytarget in the Makefile to callmake toolchainwithSKIP_INSTALLED=true
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| hack/toolchain.sh | Added skip logic for already-installed tools via new SKIP_INSTALLED flag, should-skip and go-install helper functions; refactored all tool installations to use the new helper |
| Makefile | Modified verify target to set SKIP_INSTALLED=true when calling make toolchain, preserving the original behavior of make toolchain (always reinstall) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # should-skip is a helper function to determine if installation of a tool should be skipped | ||
| # $1 is the expected command | ||
| should-skip() { | ||
| if [ "$SKIP_INSTALLED" == true ] && [ -f "$TOOL_DEST/$1" ]; then |
There was a problem hiding this comment.
Inconsistent use of single brackets with double equals operator. The codebase convention uses double brackets [[ with == for string comparisons (see hack/codegen.sh:81, hack/codegen.sh:103, hack/release/common.sh:98, etc.). Consider changing to: if [[ "$SKIP_INSTALLED" == true ]] && [ -f "$TOOL_DEST/$1" ]; then
| # Default SKIP_INSTALLED to false if not set | ||
| SKIP_INSTALLED="${SKIP_INSTALLED:=false}" | ||
|
|
||
| if [ "$SKIP_INSTALLED" == true ]; then |
There was a problem hiding this comment.
Inconsistent use of single brackets with double equals operator. The codebase convention uses double brackets [[ with == for string comparisons (see hack/codegen.sh:81, hack/codegen.sh:103, hack/release/common.sh:98, etc.). Consider changing to: if [[ "$SKIP_INSTALLED" == true ]]; then
Only the first time, right? The second time (on main, in my codespace) it takes 5s Also note that the next logical step in tooling is likely to start using |
Testing locally, on my laptop (28 cores, 64GB of memory) it's taking ~40s every time. It may have been even slower, the first time, but it's not quick.
Depends on whether |
Description
Currently,
make verifyreinstalls everything in our toolchain every time - which currently takes ~40s and will take longer when I add in thekube-api-lintingplugin togolangci-lint.This PR changes the behaviour so that
make verifywill only install tools that are missing, skipping the install step for any tool currently present.The behaviour of
make toolchainis unchanged - it will still force install the specified versions of each tool.How was this change tested?
Local tests.
Does this change impact docs?
Release Note