Repository navigation
feat(hack): rewrite check-readiness as a Go command with test suite - #2755
Conversation
Replace hack/check-readiness.sh with a Go command at cmd/check-readiness, wired into the build like cozypkg (`make check-readiness`, Version via -ldflags). The tool shells out to kubectl and reports only non-ready cozystack / Flux / Kubernetes resources parsed from status conditions, supporting --wait/--timeout, --watch, --core, --parallel, --verbose, -n/-l scoping, --no-color and --version. Add a black-box golden test suite under test/check-readiness driven by a mock kubectl over fixtures; `make test-check-readiness` runs it and it is wired into `make unit-tests`. Supersedes #2740, which added the same feature set to the bash script. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request migrates the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR replaces the Bash script ChangesCheck-readiness CLI Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request replaces the legacy check-readiness.sh bash script with a new Go-based CLI tool cmd/check-readiness for diagnosing non-ready Cozystack, Flux, and Kubernetes resources. It also introduces a robust black-box testing suite under test/check-readiness/ using mock fixtures and golden files. The review feedback focuses on critical robustness improvements: checking for the existence of kubectl in PATH to prevent silent-success failures, explicitly handling errors from kubectl executions in fetchRows and runOnce (as mandated by the style guide), safely slicing strings using runes in cutChars to avoid malformed UTF-8 sequences, and tightening column checks when parsing api-resources output.
I am having trouble creating individual review comments. Click here to see my feedback.
cmd/check-readiness/main.go (205-206)
If kubectl is not installed or not available in the system's PATH, the tool will fail to run any queries, but because errors are currently ignored, it will silently report that all resources are ready and exit with 0. Checking if kubectl is available in PATH right after parsing arguments prevents this silent-success bug.
cfg.setColors()
if _, err := exec.LookPath(cfg.kubectl[0]); err != nil {
fmt.Fprintf(os.Stderr, "Error: %s not found in PATH\n", cfg.kubectl[0])
return 2
}cmd/check-readiness/main.go (377-409)
Currently, fetchRows completely discards any errors returned by cmd.Output(). If the API server is unreachable, or if there is an authentication/authorization failure, kubectl will fail, but fetchRows will silently return an empty string. This causes the tool to report that all resources are ready. Returning the error allows the caller to handle and report it properly.
// fetchRows runs kubectl get and returns raw jsonpath output.
func (cfg *config) fetchRows(e entry) (string, error) {
scopeArgs := cfg.buildScopeArgs(e.scope)
var jsonpath string
if e.condType == "_phase" {
jsonpath = `{range .items[*]}{.metadata.namespace}{"\x1f"}{.metadata.name}{"\x1f"}{.status.phase}{"\x1f"}{""}{"\x1f"}{""}{"\x1f"}{"false"}{"\n"}{end}`
} else {
suspendField := `{"false"}`
if e.supportsSuspend {
suspendField = `{.spec.suspend}`
}
var b strings.Builder
b.WriteString(`{range .items[*]}`)
b.WriteString(`{.metadata.namespace}{"\x1f"}`)
b.WriteString(`{.metadata.name}{"\x1f"}`)
b.WriteString(`{.status.conditions[?(@.type=="` + e.condType + `")].status}{"\x1f"}`)
b.WriteString(`{.status.conditions[?(@.type=="` + e.condType + `")].reason}{"\x1f"}`)
b.WriteString(`{.status.conditions[?(@.type=="` + e.condType + `")].message}{"\x1f"}`)
b.WriteString(suspendField + `{"\n"}`)
b.WriteString(`{end}`)
jsonpath = b.String()
}
args := append([]string{}, cfg.kubectl[1:]...)
args = append(args, "get", e.kind)
args = append(args, scopeArgs...)
args = append(args, "-o", "jsonpath="+jsonpath)
cmd := exec.Command(cfg.kubectl[0], args...)
var stderr strings.Builder
cmd.Stderr = &stderr
out, err := cmd.Output()
if err != nil {
if stderr.Len() > 0 {
return "", fmt.Errorf("%s", strings.TrimSpace(stderr.String()))
}
return "", err
}
return string(out), nil
}References
- Handle errors explicitly. Discarding meaningful errors with _ is a bug. (link)
cmd/check-readiness/main.go (537-592)
Update runOnce to capture and handle the errors returned by fetchRows. If any query fails, the error is printed to the output buffer and cfg.foundIssues is set to true, ensuring the tool exits with a non-zero code and reports the failure.
func (cfg *config) runOnce() string {
cfg.foundIssues = false
cfg.ensureAPIResources()
entries := append([]entry{}, coreKinds...)
if !cfg.coreOnly {
entries = append(entries, fluxExtraKinds...)
entries = append(entries, clusterKinds...)
entries = append(entries, pvcKind)
}
rowsByIdx := make([]string, len(entries))
errsByIdx := make([]error, len(entries))
// Phase 1: fetch.
if cfg.parallel {
var wg sync.WaitGroup
for idx := range entries {
e := entries[idx]
if !cfg.kindExists(e.kind) {
continue
}
wg.Add(1)
go func(idx int, e entry) {
defer wg.Done()
rowsByIdx[idx], errsByIdx[idx] = cfg.fetchRows(e)
}(idx, e)
}
wg.Wait()
} else {
for idx := range entries {
e := entries[idx]
if !cfg.kindExists(e.kind) {
continue
}
rowsByIdx[idx], errsByIdx[idx] = cfg.fetchRows(e)
}
}
var b strings.Builder
// Phase 2: process in catalog order.
for idx := range entries {
e := entries[idx]
if !cfg.kindExists(e.kind) {
fmt.Fprintf(&b, "%s--- %s: CRD not installed, skipping%s\n", cfg.dim, e.kind, cfg.reset)
continue
}
if errsByIdx[idx] != nil {
fmt.Fprintf(&b, "%sError fetching %s: %v%s\n", cfg.red, e.kind, errsByIdx[idx], cfg.reset)
cfg.foundIssues = true
continue
}
cfg.processRows(e, rowsByIdx[idx], &b)
}
if !cfg.foundIssues {
fmt.Fprintf(&b, "%s%sAll resources are ready.%s\n", cfg.green, cfg.bold, cfg.reset)
}
return b.String()
}References
- Handle errors explicitly. Discarding meaningful errors with _ is a bug. (link)
cmd/check-readiness/main.go (432-439)
The comment states 'Use runes to be safe', but the implementation uses byte-based slicing (s[:n]). Slicing a string by bytes can cut in the middle of a multi-byte UTF-8 character, resulting in invalid UTF-8 strings. Converting the string to a slice of runes before slicing ensures safety and correctness.
// cutChars mimics `cut -c1-N` (rune-based to safely handle multi-byte UTF-8 characters).
func cutChars(s string, n int) string {
runes := []rune(s)
if len(runes) <= n {
return s
}
return string(runes[:n])
}
cmd/check-readiness/main.go (359-361)
Since kubectl api-resources output always contains at least 4 columns (NAME, APIVERSION, NAMESPACED, KIND), checking len(fields) < 4 is more robust and prevents potential out-of-bounds or logical errors.
if len(fields) < 4 {
continue
}
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — clean Go rewrite of the readiness diagnostic with a thorough black-box golden suite; builds and vets clean, the parallel path is race-free, and there is no command-injection surface.
Business context: replace the hack/check-readiness.sh heuristic script with a cmd/check-readiness Go tool that reports non-ready cozystack/Flux/Kubernetes resources from status conditions, built and tested like the other in-repo commands.
Verified: go vet and go build are clean; kubectl is invoked via exec.Command(cfg.kubectl[0], args...) (arg slice, no shell), so the -n/-l passthrough is not an injection vector; the --parallel path fills a pre-sized rowsByIdx slice by unique index behind a WaitGroup barrier and mutates shared state (foundIssues) only in the sequential phase-2 pass, and go test -race ./test/check-readiness/ passes (30 golden scenarios plus a PTY color test). The Makefile wires check-readiness (with the Version ldflag) and test-check-readiness into unit-tests. The removed shell script has no remaining functional references — the only hits are historical v1.3.0 changelog entries, correctly left untouched.
Non-blocking: this supersedes #2740 (the bash-side version of the same feature set), so #2740 should be closed once this lands.
What this PR does
Replaces the
hack/check-readiness.shdiagnostic with a Go command atcmd/check-readiness, integrated into the build the same way ascozypkg:make check-readinessbuilds_out/bin/check-readiness, withVersioninjected via
-ldflags.kubectland reports only non-ready cozystack /Flux / Kubernetes resources, parsed from status conditions (not column
heuristics). Flags:
--wait/--timeout,--watch,--core,--parallel,--verbose,-n/-lscoping,--no-color,--version.Adds a black-box golden test suite under
test/check-readiness/:kubectlserves canned fixtures; the suite diffs stdout/stderr/exitagainst golden files across 30 scenarios (arg parsing, condition/phase/suspend
handling, CRD-not-installed, kubectl arg propagation, wait/timeout, edge
cases) plus a PTY test for color output.
make test-check-readinessruns it; wired intomake unit-tests.Supersedes #2740, which added the same feature set to the bash script.
Release note
Summary by CodeRabbit
Release Notes
New Features
check-readinesscommand for verifying Kubernetes resource readiness status across namespacesTests