ci(operator): centralize Kubernetes test versions#281
Conversation
github-actions
Bot
added
doc
AnouarMohamed
requested review from
ayuskauskas,
lockwobr and
rice-riley
as code owners
📝 WalkthroughWalkthroughThis PR introduces Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/operator-ci.yaml:
- Around line 68-84: Add a permissions block to the k8s-test-versions job to
enforce least privilege access. Since this job only checks out the repository
and runs a make command to print version information with no write or deployment
operations, add explicit minimal permissions (such as read-all or restricted
permissions) at the job level to reduce the attack surface if the job were
compromised.
In `@operator/Makefile`:
- Around line 166-179: The validate-kind-node-image target hardcodes docker as
the first choice for container command checking, which ignores the DOCKER_CMD
variable that may be configured to use podman instead. Update the conditional
logic to check for the DOCKER_CMD variable first before falling back to docker,
then podman, and finally curl. This ensures that if DOCKER_CMD is explicitly set
to podman, that preference is honored during image validation rather than
defaulting to docker if it exists on the system.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8746bd17-b760-4ccd-be48-1ad24591a61a
📒 Files selected for processing (13)
.github/workflows/agent-ci.yaml.github/workflows/operator-ci.yamldocs/README.mddocs/development.mddocs/kubernetes-support.mdoperator/Makefileoperator/README.mdoperator/api/v1alpha1/webhook_suite_test.gooperator/config/local-dev/ctlptl-config.yamloperator/deps.mkoperator/internal/controller/suite_test.gooperator/internal/testenv/envtest.gooperator/k8s-test-versions.mk
Signed-off-by: AnouarMohamed <m.anouar@mundiapolis.ma>
AnouarMohamed
force-pushed
the
fix/issue-238-k8s-test-versions
branch
from
d0d2e08 to
9d8e057
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/internal/testenv/envtest.go`:
- Around line 39-49: The function `BinaryAssetsDirectory` returns bare errors
from `findOperatorRoot()` and `makeDefault()` calls without wrapping them with
context. Wrap both error returns using `fmt.Errorf` with the `%w` verb to
provide diagnostic context, following the pattern `fmt.Errorf("descriptive
message: %w", err)` for each of the two return statements that currently return
bare `err` values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 19a32c2d-a2cb-4982-a8b7-691b971b7a57
📒 Files selected for processing (13)
.github/workflows/agent-ci.yaml.github/workflows/operator-ci.yamldocs/README.mddocs/development.mddocs/kubernetes-support.mdoperator/Makefileoperator/README.mdoperator/api/v1alpha1/webhook_suite_test.gooperator/config/local-dev/ctlptl-config.yamloperator/deps.mkoperator/internal/controller/suite_test.gooperator/internal/testenv/envtest.gooperator/k8s-test-versions.mk
| operatorRoot, err := findOperatorRoot() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| version := strings.TrimPrefix(os.Getenv(envtestK8SVersionVar), "v") | ||
| if version == "" { | ||
| version, err = makeDefault(operatorRoot, envtestK8SVersionVar) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add context when returning helper errors from BinaryAssetsDirectory.
At Line 41 and Line 48, this function returns bare err. Wrap both returns with %w context so failures are diagnosable at call sites.
Suggested diff
operatorRoot, err := findOperatorRoot()
if err != nil {
- return "", err
+ return "", fmt.Errorf("resolving operator root for envtest assets: %w", err)
}
version := strings.TrimPrefix(os.Getenv(envtestK8SVersionVar), "v")
if version == "" {
version, err = makeDefault(operatorRoot, envtestK8SVersionVar)
if err != nil {
- return "", err
+ return "", fmt.Errorf("resolving %s default: %w", envtestK8SVersionVar, err)
}
}As per coding guidelines, “Wrap errors with fmt.Errorf("…: %w", err) using %w for error wrapping; never return bare err.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/internal/testenv/envtest.go` around lines 39 - 49, The function
`BinaryAssetsDirectory` returns bare errors from `findOperatorRoot()` and
`makeDefault()` calls without wrapping them with context. Wrap both error
returns using `fmt.Errorf` with the `%w` verb to provide diagnostic context,
following the pattern `fmt.Errorf("descriptive message: %w", err)` for each of
the two return statements that currently return bare `err` values.
Source: Coding guidelines
1 participant
Summary
operator/k8s-test-versions.mkas the owner for envtest, Kind node image, Kind binary, and CI matrix versionskindest/nodetags before Kind cluster creationBinaryAssetsDirectoryin Go suites from the centralized version owner instead of hard-coded pathsNotes
upstream/mainalready uses envtest1.36.0while local/CI Kind node images remain on1.35.0. This keeps those current values and documents the intentional split because Kind does not publish every Kubernetes patch version.Closes #238.
Validation
make -C operator print-k8s-test-versions-github-outputmake -C operator envtestmake -C operator render-ctlptl-configmake -C operator validate-kind-node-imagemake -C operator validate-kind-node-image DOCKER_CMD=podmanwith a temporary Podman shim to confirmDOCKER_CMDis preferredmake -C operator -n create-kind-clustermake -C operator -n create-deployment-policy-clustercd operator && make unit-tests(first run hit an envtest1.36.0startup timeout; retry passed)git diff --check