docs: document running tests when Docker credential helper fails (e.g. WSL) #940
Conversation
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
WalkthroughDocumentation was added to two files explaining how to run tests when Docker credential helper failures occur, particularly in WSL environments. This includes Docker configuration fixes and alternative commands for running unit and integration tests without Docker. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 54-58: Reword the unit test instruction to clarify that the short
test command excludes envtest-dependent packages (e.g., change to "Unit tests
(excludes envtest-dependent packages): go test ./pkg/... ./cmd/... -count=1
-short") and replace the hardcoded "1.34.0" in the bin/setup-envtest example
with a placeholder or reference to the ENVTEST_K8S_VERSION value from
build/makefile/testenv.mk (e.g., use "<VERSION>" or a note to substitute
ENVTEST_K8S_VERSION) so the example uses the canonical version variable instead
of a literal; update the example invocation that calls bin/setup-envtest to
reflect this placeholder.
- Around line 49-51: The current step that runs `echo '{}' >
~/.docker/config.json` dangerously overwrites the entire Docker config; instead
preserve other keys by updating the instruction to remove only the credsStore
field (keep the existing backup step `cp ~/.docker/config.json
~/.docker/config.json.bak`) and either: (a) use a safe JSON-editing tool (e.g.,
jq) to delete the top-level "credsStore" property and write the result back to
~/.docker/config.json, or (b) instruct users to manually remove the "credsStore"
line from their config and verify important keys like "auths", "proxies", and
"experimental" are retained; replace the destructive `echo '{}' >
~/.docker/config.json` line in AGENTS.md accordingly.
In `@docs/developer/building-from-source.md`:
- Around line 54-58: Update the unit test line to clarify intent by explaining
it runs only unit tests and skips envtest-dependent integration tests (e.g.,
change the phrasing around `go test ./pkg/... ./cmd/... -count=1 -short` to say
"Unit tests only (skips envtest integration tests)"). Also replace the hardcoded
`1.34.0` in the envtest example with a placeholder like `<ENVTEST_K8S_VERSION>`
and add a short note to fetch the actual version from `ENVTEST_K8S_VERSION` in
`build/makefile/testenv.mk` when running the `make envtest` /
`bin/setup-envtest` commands so the docs stay correct if the Makefile version
changes.
🧹 Nitpick comments (1)
docs/developer/building-from-source.md (1)
62-70: Version reference table is helpful; consider simplifying Go/builder row.The table clearly documents version sources, which is valuable. The Go/builder row references two locations (
build/makefile/golang.mkandbuild/builder/Dockerfile), which may require users to check multiple files. If both always contain the same version, consider noting that or pointing to a single authoritative source.
| cp ~/.docker/config.json ~/.docker/config.json.bak | ||
| echo '{}' > ~/.docker/config.json | ||
| ``` |
There was a problem hiding this comment.
Risk: Docker config overwrite may lose other settings beyond credsStore.
The command echo '{}' > ~/.docker/config.json replaces the entire Docker configuration file, not just the credsStore field. Users may have other important settings in this file (e.g., auths for registry credentials, proxies, experimental flags, detachKeys, etc.) that will be lost.
🛡️ Safer alternative using jq
-cp ~/.docker/config.json ~/.docker/config.json.bak
-echo '{}' > ~/.docker/config.json
+cp ~/.docker/config.json ~/.docker/config.json.bak
+jq 'del(.credsStore)' ~/.docker/config.json > ~/.docker/config.json.tmp && mv ~/.docker/config.json.tmp ~/.docker/config.jsonThis preserves other Docker settings while removing only the problematic credsStore field. Alternatively, document that users should manually edit the file to remove just the credsStore line, or verify they have no other important settings before using the destructive approach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cp ~/.docker/config.json ~/.docker/config.json.bak | |
| echo '{}' > ~/.docker/config.json | |
| ``` | |
| cp ~/.docker/config.json ~/.docker/config.json.bak | |
| jq 'del(.credsStore)' ~/.docker/config.json > ~/.docker/config.json.tmp && mv ~/.docker/config.json.tmp ~/.docker/config.json |
🤖 Prompt for AI Agents
In `@AGENTS.md` around lines 49 - 51, The current step that runs `echo '{}' >
~/.docker/config.json` dangerously overwrites the entire Docker config; instead
preserve other keys by updating the instruction to remove only the credsStore
field (keep the existing backup step `cp ~/.docker/config.json
~/.docker/config.json.bak`) and either: (a) use a safe JSON-editing tool (e.g.,
jq) to delete the top-level "credsStore" property and write the result back to
~/.docker/config.json, or (b) instruct users to manually remove the "credsStore"
line from their config and verify important keys like "auths", "proxies", and
"experimental" are retained; replace the destructive `echo '{}' >
~/.docker/config.json` line in AGENTS.md accordingly.
| - **Without Docker:** Unit: `go test ./pkg/... ./cmd/... -count=1 -short` (exit 1 if envtest packages fail). Integration: use `ENVTEST_K8S_VERSION` from `build/makefile/testenv.mk` (not `latest`): | ||
| ```bash | ||
| make envtest | ||
| KUBEBUILDER_ASSETS="$(bin/setup-envtest use 1.34.0 -p path --bin-dir bin)" go test ./pkg/... -timeout 30m | ||
| ``` |
There was a problem hiding this comment.
Clarify "exit 1" expectation and consider version variable.
Two points:
-
The phrase "exit 1 if envtest packages fail" may confuse users—why document a command expected to partially fail? Consider rephrasing to: "Unit tests (excludes envtest-dependent packages):
go test ./pkg/... ./cmd/... -count=1 -short" or clarify that-shortskips integration tests that require envtest. -
The example hardcodes version
1.34.0even though line 54 instructs users to checkENVTEST_K8S_VERSIONfrombuild/makefile/testenv.mk. This could lead to version drift if the Makefile is updated but documentation is not. Consider using a placeholder like<VERSION>with a note to substitute the actual value, or accept the maintenance trade-off.
🤖 Prompt for AI Agents
In `@AGENTS.md` around lines 54 - 58, Reword the unit test instruction to clarify
that the short test command excludes envtest-dependent packages (e.g., change to
"Unit tests (excludes envtest-dependent packages): go test ./pkg/... ./cmd/...
-count=1 -short") and replace the hardcoded "1.34.0" in the bin/setup-envtest
example with a placeholder or reference to the ENVTEST_K8S_VERSION value from
build/makefile/testenv.mk (e.g., use "<VERSION>" or a note to substitute
ENVTEST_K8S_VERSION) so the example uses the canonical version variable instead
of a literal; update the example invocation that calls bin/setup-envtest to
reflect this placeholder.
| - Unit tests: `go test ./pkg/... ./cmd/... -count=1 -short` (envtest-dependent packages may fail; exit 1 is expected). | ||
| - Integration (envtest): use `ENVTEST_K8S_VERSION` from `build/makefile/testenv.mk` (e.g. 1.34.0; do not use `latest`): | ||
| ```sh | ||
| make envtest | ||
| KUBEBUILDER_ASSETS="$(bin/setup-envtest use 1.34.0 -p path --bin-dir bin)" go test ./pkg/... -timeout 30m |
There was a problem hiding this comment.
Clarify unit test expectations and version hardcoding.
Two minor points:
-
"exit 1 is expected": This phrasing suggests the command will fail, which may confuse users. Consider clarifying that
-shortskips integration tests requiring envtest, so some packages won't be tested: "Unit tests only (skips envtest integration tests):go test ./pkg/... ./cmd/... -count=1 -short". -
Hardcoded version
1.34.0: Line 55 directs users tobuild/makefile/testenv.mkforENVTEST_K8S_VERSION, yet the example hardcodes1.34.0. If the Makefile is updated, this documentation will become stale. Consider using<VERSION>as a placeholder, or accept the maintenance cost for the sake of a working copy-paste example.
🤖 Prompt for AI Agents
In `@docs/developer/building-from-source.md` around lines 54 - 58, Update the unit
test line to clarify intent by explaining it runs only unit tests and skips
envtest-dependent integration tests (e.g., change the phrasing around `go test
./pkg/... ./cmd/... -count=1 -short` to say "Unit tests only (skips envtest
integration tests)"). Also replace the hardcoded `1.34.0` in the envtest example
with a placeholder like `<ENVTEST_K8S_VERSION>` and add a short note to fetch
the actual version from `ENVTEST_K8S_VERSION` in `build/makefile/testenv.mk`
when running the `make envtest` / `bin/setup-envtest` commands so the docs stay
correct if the Makefile version changes.
|
Hi @rich7420, |
2 participants
Description
Document how to run tests when Docker works and when
make testfails (e.g. WSL + Docker Desktop credential helper).make test; Option 1: fix Docker config (backup/clear~/.docker/config.json); Option 2: run without Docker (unit, envtest, validate). Version reference table (envtest K8s, envtest tool, Go/builder) — use Makefile values, notlatest.Related Issues
Closes #939
Checklist
Breaking Changes
Additional Notes
Summary by CodeRabbit
Documentation