Simplify Makefile and tool installations#355
Conversation
📝 WalkthroughWalkthroughRefactors build tooling to use ABS_BUILD_DIR/ABS_TOOLS_DIR, adds a new downloader script (hack/uget.sh) with checksum support, replaces many go-installs with uget fetches, removes hack/go-install.sh, updates tool wiring and go.mod, and adjusts docs and Makefiles for Dex and Go version changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Make as Makefile
participant UGET as hack/uget.sh
participant Net as Network
participant CS as hack/tools.checksums
participant Tools as ABS_TOOLS_DIR
rect rgb(248,248,255)
Note over Make,UGET: New tool fetch flow
Make->>UGET: request tool + version + params
UGET->>Net: resolve & download artifact URL
Net-->>UGET: binary/archive
UGET->>CS: lookup expected checksum
CS-->>UGET: checksum
UGET->>UGET: verify checksum
alt checksum matches
UGET->>Tools: place binary
Tools-->>Make: tool available
else checksum mismatch
UGET-->>Make: error (checksum mismatch)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xrstf
force-pushed
the
uget
branch
4 times, most recently
from
965c132 to
9eaa7b0
Compare
On-behalf-of: @SAP christoph.mewes@sap.com
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Makefile (2)
20-20: Consider removing trailing slash from BUILD_DIR for consistency.Line 20 defines
BUILD_DIR=bin/with a trailing slash, but most usages in the Makefile assume no trailing slash (e.g.,$(ABS_BUILD_DIR)/$(KUBE_CLIENT_GEN_BIN)). While this may work due to path normalization, removing the trailing slash would improve consistency.Apply this diff:
-BUILD_DIR=bin/ +BUILD_DIR=bin
269-278: Consider improving error handling in dex build process.Line 275 uses
|| trueto ignore git clone errors, which could mask legitimate failures (network issues, permissions). The intent appears to be allowing re-runs, but this also hides real errors. Consider checking if the directory exists before cloning, or removing the directory first.Apply this diff for better error handling:
DEX_TMP_DIR = $(TOOLS_DIR)/dex-clone-$(DEX_VER) $(DEX): mkdir -p $(TOOLS_DIR) - git clone --branch $(DEX_VER) --depth 1 https://github.com/dexidp/dex $(DEX_TMP_DIR) || true + rm -rf $(DEX_TMP_DIR) + git clone --branch $(DEX_VER) --depth 1 https://github.com/dexidp/dex $(DEX_TMP_DIR) cd $(DEX_TMP_DIR) && GOWORK=off make build cp -a $(DEX_TMP_DIR)/bin/dex $(DEX) rm -rf $(DEX_TMP_DIR)hack/uget.sh (1)
317-320: GOBIN setting may be redundant in Go build command.Line 320 sets
GOBIN=$(realpath .)but then uses the-o "$BINARY"flag which explicitly specifies the output location. The GOBIN setting may be unnecessary since-otakes precedence.Consider simplifying:
- GOFLAGS=-trimpath GOARCH="$arch" GOOS="$os" GOBIN=$(realpath .) $UGET_GO_BUILD_CMD -o "$BINARY" "$url" + GOFLAGS=-trimpath GOARCH="$arch" GOOS="$os" $UGET_GO_BUILD_CMD -o "$BINARY" "$url"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
Makefile(10 hunks)contrib/kcp/Makefile(2 hunks)docs/content/contributing/index.md(1 hunks)docs/content/setup/kcp-setup.md(2 hunks)docs/content/setup/quickstart.md(3 hunks)go.mod(0 hunks)hack/go-install.sh(0 hunks)hack/tools.checksums(1 hunks)hack/tools.go(0 hunks)hack/uget.sh(1 hunks)
💤 Files with no reviewable changes (3)
- hack/go-install.sh
- hack/tools.go
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T09:05:29.762Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.762Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
docs/content/setup/quickstart.md
🪛 checkmake (0.2.2)
Makefile
[warning] 134-134: Target body for "build" exceeds allowed length of 5 (6).
(maxbodylength)
🔇 Additional comments (17)
docs/content/setup/quickstart.md (1)
3-3: LGTM! Documentation updates align with new build structure.The changes correctly reflect the new tool paths and improve documentation consistency with the title fix and Kubernetes capitalization.
Also applies to: 38-38, 48-48, 50-51
docs/content/setup/kcp-setup.md (1)
38-39: LGTM! Dex setup simplified with new build workflow.The changes correctly reflect the new
make dextarget and the relocated binary athack/tools/dex.Also applies to: 55-56
contrib/kcp/Makefile (2)
18-26: LGTM! Build directory structure and PATH reordering are correct.The new variables properly separate build artifacts (ABS_BUILD_DIR) from downloaded tools (ABS_TOOLS_DIR), and PATH reordering ensures local tools take precedence over system tools for reproducible builds.
40-41: LGTM! UGET invocation for Go modules is correct.The
GO_MODULE=trueflag correctly indicates this tool should be fetched as a Go module, and the version parameter is properly specified.Makefile (5)
130-131: LGTM! Clean target appropriately removes build and tool directories.The new clean target correctly removes both build artifacts and downloaded tools, providing a complete cleanup mechanism.
182-187: LGTM! Controller-gen download correctly uses UNCOMPRESSED flag and binary pattern.The target properly specifies
UNCOMPRESSED=truefor the single-file download and provides a binary pattern to handle the wildcard match.
241-246: LGTM! verify_boilerplate.py download uses correct parameters.The download uses
UNCOMPRESSED=truefor the raw Python script and specifies the exact filename as the binary pattern.
249-250: LGTM! Boilerplate verification correctly skips third-party code.The target appropriately skips the dex directory and hack/uget.sh (which has a different license header as noted in the file).
61-61: No issues found with golangci-lint version format.The version format at line 61 is correct. The URL pattern prepends 'v' to the version number in the download path, and verification confirms the constructed URL for version 2.1.6 is accessible. This format is appropriate for golangci-lint releases.
hack/uget.sh (6)
16-58: LGTM! Configuration variables are well-documented and have sensible defaults.The configuration section clearly documents all environment variables and provides appropriate default values.
161-275: LGTM! URL construction and placeholder replacement logic is well-structured.The URL building functions correctly handle placeholder detection, replacement from system data or provided key-value pairs, and support both live and predetermined replacements. The sorting and deduplication of placeholders ensures consistent checksum keys.
357-417: LGTM! Install and update functions correctly handle versioning and checksums.The functions properly track installed versions, verify checksums before installation, and support batch checksum updates for all known variants. The version file mechanism prevents unnecessary re-downloads.
462-475: Update mode logic correctly processes all known variants.The update mode iterates through all known checksums for the binary, downloads each variant, recalculates checksums, and updates the checksum file. Note that line 472 has the same awk injection vulnerability mentioned in an earlier comment.
Verify this comment references the awk injection issue flagged earlier at lines 120-126.
485-488: Verify GOARCH/GOOS addition for Go modules is always correct.Lines 486-488 automatically add GOARCH and GOOS to the kvString for Go modules to support cross-architecture checksums. Confirm this behavior is always desired, even when the module URL pattern doesn't include architecture placeholders.
347-350: Review comment is inapplicable to this codebase.The concern about malicious path components in
BINARY_PATTERNdoes not apply here. All uses ofBINARY_PATTERNin the Makefile pass hardcoded strings (e.g.,controller-gen*,gotestsum) defined as static Makefile variables. The 4th argument touget.shis not exposed to external or user-controlled input—it originates exclusively from hardcoded binary name variables. Additionally, the operation executes within an isolated temporary directory, further containing any potential risk.Likely an incorrect or invalid review comment.
hack/tools.checksums (1)
1-7: Verify that checksums were refreshed via the documented update procedure.The checksums file format is valid and correctly structured. However, checksum accuracy cannot be verified through format validation alone. Confirm that you ran
UGET_UPDATE=true make ...to regenerate these checksums based on the current tool versions before merging.docs/content/contributing/index.md (1)
20-20: No issues found. Documentation is accurate.Go 1.24 was released on February 11, 2025. The codebase confirms this requirement consistently across all go.mod files, which specify
go 1.24.0for the main module, SDK, CLI, documentation generators, and contrib modules. The documentation statement aligns with the project's explicit Go version requirement.
|
|
||
| ```bash | ||
| ./bin/dex serve examples/config-dev.yaml | ||
| ./hack/tools/dex serve examples/config-dev.yaml |
There was a problem hiding this comment.
| ./hack/tools/dex serve examples/config-dev.yaml | |
| make run-dex |
I don't think examples/ exists, or?
Anyhow the run-dex recipe could be used instead.
There was a problem hiding this comment.
Good point.
run-dex however uses our own internal development config, whereas the docs mean to instruct the user to use dex's config files. However since the make dex command now builds the dex binary and then deletes the temporary clone, that config-dev.yaml (https://github.com/dexidp/dex/blob/master/examples/config-dev.yaml) is not available anymore.
What would you suggest? Revert the docs to make people continue to manually clone and build and not delete dex, or copy dex's config-dev.yaml into our repo? Or copy it too when building dex (i.e. make our Makefile copy the file)?
There was a problem hiding this comment.
Mhm.. I think we can handwave the dex configuration and just use the development config.
When people implement kube-bind in their environment they will have to configure and use their OIDC provider anyhow, explicitly requiring the user to configure dex doesn't really add anything IMHO.
On-behalf-of: @SAP christoph.mewes@sap.com
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hack/uget.sh (1)
173-178: Consider using fixed-string grep to prevent regex injection.The
valueFromPairandsetKeyInPairsfunctions use extended regex (-E) with$keyvariable interpolation. Although current usage passes only controlled placeholder names, using fixed-string mode (grep -F) would be more defensive and prevent potential regex injection if these functions are ever called with untrusted input.uget::url::valueFromPair() { local kvString="$1" local key="$2" - (echo "$kvString" | grep -oE ";$key=([^;]+)" || true) | cut -f2 -d'=' + (echo "$kvString" | grep -oF ";$key=" || true) | sed 's/^;//' | sed 's/=.*//' | head -1 | xargs -I {} sh -c 'echo "$kvString" | sed -n "s/.*;'$key'=\([^;]*\).*/\1/p"' || true)Or more simply, parse the kvString directly without grep:
uget::url::valueFromPair() { local kvString="$1" local key="$2" - (echo "$kvString" | grep -oE ";$key=([^;]+)" || true) | cut -f2 -d'=' + [[ "$kvString" =~ ;${key}=([^;]+) ]] && echo "${BASH_REMATCH[1]}" }Also applies to: 180-193
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/uget.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: go-test
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: verify
🔇 Additional comments (3)
hack/uget.sh (3)
120-126: Security fix for awk variable injection is correctly applied.The previous review flagged shell/awk injection vulnerabilities where
$BINARYand$kvStringwere interpolated directly into awk patterns. The current code properly uses awk's-vflag to safely pass these values as variables while using single-quoted awk scripts. This prevents injection attacks and correctly remediates the prior concern.Also applies to: 145-145, 472-472
16-16: Solid error handling and resource management practices.The script correctly uses
set -euo pipefailfor strict execution, creates and cleans up temporary directories consistently, maintains atomic checksum file updates, and applies proper quoting throughout. Resource cleanup code properly usesrm -rf --to prevent flag interpretation. Error messages provide adequate context for troubleshooting.Also applies to: 63-80, 137-154, 382-391
306-320: Go module cross-compilation approach is well-justified.The use of a dummy module with
go mod initandgo getto enable cross-compilation of Go binaries is a correct workaround for Go's security restriction documented in golang/go#57485. The explicit GOARCH/GOOS environment variables and the comment explaining the rationale demonstrate solid understanding of Go's build constraints.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/content/setup/local-setup-with-kind.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T06:41:37.475Z
Learnt from: mjudeikis
Repo: kube-bind/kube-bind PR: 342
File: docs/content/usage/migration.md:34-37
Timestamp: 2025-10-22T06:41:37.475Z
Learning: In the kube-bind repository documentation and examples, "mangodb" (with group "mangodb.com" and resource "mangodbs") is an intentional fictional example name chosen to avoid referencing real products like MongoDB. It should not be treated as a typo.
Applied to files:
docs/content/setup/local-setup-with-kind.md
🪛 LanguageTool
docs/content/setup/local-setup-with-kind.md
[grammar] ~7-~7: Use a hyphen to join words.
Context: ...be-bind/backend * Provides kube-bind compatible backend for MangoDB resources...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/content/setup/local-setup-with-kind.md
6-6: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: go-test
- GitHub Check: verify
- GitHub Check: go-test-e2e
On-behalf-of: @SAP christoph.mewes@sap.com
On-behalf-of: @SAP christoph.mewes@sap.com
2 participants
Summary
This introduces µget to download build tools and replace the
go-install.sh. µget is better because it can download prebuilt binaries and handles checksums to detect malicious upstream changes. For a developer, nothing really changes in the workflows ("make lint" works just as before), only when you update a tool will you have to runUGET_UPDATE=true make .......to make µget update all checksums.Besides this, this PR also performs some general cleanup to our Makefiles:
mktemp -dfor eachmakeinvocationGOBIN_DIRwas renamed toBUILD_DIR, becauseTOOLS_GOBIN_DIRwas confusing. Now it'sTOOLS_DIRandBUILD_DIR, plus theirABS_...variants.INSTALL_GOBINlogic was removed, since it was also unused.bin/, but intohack/tools/, clearly separating them from our own build artifacts.tools.gowas thinned out a lot. Since we already, even before this PR, installed most these tools using a fake module with versions sourced from our Makefile, having synthetic Go dependencies in kube-bind makes no sense (it would only make sense if somewhere we did an unversionedgo install k8s.code-generator/cmd/lister-gen) except for the kube codegen where we refer to its kube_codegen.sh bygo listing the module.make cleanwas added to removeBUILD_DIRandTOOLS_DIR.What Type of PR Is This?
/kind cleanup
Release Notes
Summary by CodeRabbit
Chores
Documentation