dep: controller runtime v0.23.3#296
Conversation
KevFan
force-pushed
the
controller-runtime
branch
from
12270a0 to
0e7c369
Compare
KevFan
force-pushed
the
controller-runtime
branch
from
0e7c369 to
dea91a5
Compare
📝 WalkthroughWalkthroughThe pull request updates build tooling to compute Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Signed-off-by: KevFan <kevin_fan@hotmail.co.uk>
KevFan
force-pushed
the
controller-runtime
branch
from
dea91a5 to
0053523
Compare
|
/lgtm |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/reconcilers/authorino_reconciler_test.go (1)
128-136:⚠️ Potential issue | 🟠 MajorAdd TypeMeta to all Deployment fixtures for Server-Side Apply consistency.
The
existingServicefixture includes TypeMeta (lines 73–76) to support Server-Side Apply operations, but all threeexistingDeploymentfixtures (lines 128–136, 165–175, and 202–212) lack this initialisation. SincereconcileDeploymentcallsreconcileResource, which invokesApplyResourceusingclient.Apply(Server-Side Apply), the Deployment fixtures require TypeMeta for proper operation and consistency.Update all three fixtures to include TypeMeta following the same pattern as the Service fixture:
Suggested fix for all three existingDeployment fixtures
existingDeployment := &appsv1.Deployment{ TypeMeta: metav1.TypeMeta{ Kind: "Deployment", APIVersion: appsv1.SchemeGroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ Name: "test-authorino", Namespace: namespace, }, Spec: appsv1.DeploymentSpec{ Replicas: pointer.Int32(1), }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reconcilers/authorino_reconciler_test.go` around lines 128 - 136, The three Deployment test fixtures named existingDeployment used by reconcileDeployment (which calls reconcileResource -> ApplyResource -> client.Apply) are missing TypeMeta required for Server-Side Apply; update each existingDeployment fixture to include a TypeMeta block with Kind "Deployment" and APIVersion set to appsv1.SchemeGroupVersion.String(), mirroring the existingService fixture pattern so client.Apply receives proper TypeMeta for SSA operations.
🧹 Nitpick comments (1)
Makefile (1)
145-146: Consider adding a fallback for robustness.The dynamic computation from controller-runtime version is a good improvement that addresses the unpinning requirement from PR
#295. However, ifgo listfails (e.g., beforego mod download), the awk command produces empty output, leavingENVTEST_VERSIONempty.Consider adding a fallback:
🔧 Suggested fallback pattern
# ENVTEST_VERSION is the version of controller-runtime release branch to fetch the envtest setup script (i.e. release-0.16) -ENVTEST_VERSION ?= $(shell go list -m -f "{{ .Version }}" sigs.k8s.io/controller-runtime 2>/dev/null | awk -F'[v.]' '{printf "release-%d.%d", $$2, $$3}') +ENVTEST_VERSION ?= $(or $(shell go list -m -f "{{ .Version }}" sigs.k8s.io/controller-runtime 2>/dev/null | awk -F'[v.]' '{printf "release-%d.%d", $$2, $$3}'),release-0.19)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 145 - 146, The computed ENVTEST_VERSION can be empty if `go list` fails; update the `ENVTEST_VERSION` assignment so it falls back to a sensible default (e.g., "release-0.16") when the shell pipeline `go list -m -f "{{ .Version }}" sigs.k8s.io/controller-runtime 2>/dev/null | awk -F'[v.]' '{printf "release-%d.%d", $$2, $$3}'` produces no output — implement the fallback using a shell OR or parameter expansion so ENVTEST_VERSION gets the computed value when available and the default when not, while preserving the existing variable name ENVTEST_VERSION and behavior when controller-runtime is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/crd/bases/operator.authorino.kuadrant.io_authorinos.yaml`:
- Around line 97-103: The CRD allows certSecretRef.name to default to an empty
string which can produce runtime failures when CertSecret.Name is used without
guarding; update the reconciler code to validate non-empty secret names before
using them: in controllers/authorino_controller.go (where CertSecret is checked)
add an explicit check that CertSecret.Name != "" before attempting lookups or
usage, and in pkg/reconcilers/deployment.go (the deployment reconciler paths
that build Secret volume mounts / volume references) add guards that ensure
certSecretRef.Name (or CertSecret.Name) is non-empty and return a clear
reconcile error if empty so the controller does not create invalid volume secret
references. Ensure all code paths that read CertSecret.Name perform the
non-empty check first.
---
Outside diff comments:
In `@pkg/reconcilers/authorino_reconciler_test.go`:
- Around line 128-136: The three Deployment test fixtures named
existingDeployment used by reconcileDeployment (which calls reconcileResource ->
ApplyResource -> client.Apply) are missing TypeMeta required for Server-Side
Apply; update each existingDeployment fixture to include a TypeMeta block with
Kind "Deployment" and APIVersion set to appsv1.SchemeGroupVersion.String(),
mirroring the existingService fixture pattern so client.Apply receives proper
TypeMeta for SSA operations.
---
Nitpick comments:
In `@Makefile`:
- Around line 145-146: The computed ENVTEST_VERSION can be empty if `go list`
fails; update the `ENVTEST_VERSION` assignment so it falls back to a sensible
default (e.g., "release-0.16") when the shell pipeline `go list -m -f "{{
.Version }}" sigs.k8s.io/controller-runtime 2>/dev/null | awk -F'[v.]' '{printf
"release-%d.%d", $$2, $$3}'` produces no output — implement the fallback using a
shell OR or parameter expansion so ENVTEST_VERSION gets the computed value when
available and the default when not, while preserving the existing variable name
ENVTEST_VERSION and behavior when controller-runtime is present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10bac99c-8df3-462d-9f3a-8f6fcc6e52ff
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
Makefilebundle/manifests/authorino-operator.clusterserviceversion.yamlbundle/manifests/operator.authorino.kuadrant.io_authorinos.yamlcharts/authorino-operator/templates/manifests.yamlconfig/crd/bases/operator.authorino.kuadrant.io_authorinos.yamlconfig/deploy/manifests.yamlconfig/install/manifests.yamlgo.modpkg/reconcilers/authorino_reconciler_test.go
| default: "" | ||
| description: |- | ||
| Name of the referent. | ||
| This field is effectively required, but due to backwards compatibility is | ||
| allowed to be empty. Instances of this type with an empty value here are | ||
| almost certainly wrong. | ||
| More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: confirm whether empty CertSecret.Name is guarded before use.
rg -nP --type=go -C4 'CertSecret\s*==\s*nil|CertSecret\.Name|namespacedName\(|GetTlsVolume\(' controllers/authorino_controller.go pkg/reconcilers/deployment.goRepository: Kuadrant/authorino-operator
Length of output: 3483
Avoid defaulting TLS secret names to empty strings without runtime validation.
Setting default: "" on certSecretRef.name permits schema-valid objects that will fail at runtime. The controller checks whether CertSecret is nil (line 178) but does not validate whether Name is empty before using it. The deployment reconciler (lines 103, 110) accesses CertSecret.Name directly with no guard, which would create invalid volume secret references if the name is empty.
If backwards compatibility requires accepting empty values, add explicit name != "" validation in reconciler paths before secret lookup and volume creation (controllers/authorino_controller.go line 185–186 and pkg/reconcilers/deployment.go lines 103–105, 110–112).
Also applies to: 138–143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/crd/bases/operator.authorino.kuadrant.io_authorinos.yaml` around lines
97 - 103, The CRD allows certSecretRef.name to default to an empty string which
can produce runtime failures when CertSecret.Name is used without guarding;
update the reconciler code to validate non-empty secret names before using them:
in controllers/authorino_controller.go (where CertSecret is checked) add an
explicit check that CertSecret.Name != "" before attempting lookups or usage,
and in pkg/reconcilers/deployment.go (the deployment reconciler paths that build
Secret volume mounts / volume references) add guards that ensure
certSecretRef.Name (or CertSecret.Name) is non-empty and return a clear
reconcile error if empty so the controller does not create invalid volume secret
references. Ensure all code paths that read CertSecret.Name perform the
non-empty check first.
Signed-off-by: KevFan <kevin_fan@hotmail.co.uk>
KevFan
changed the title
KevFan
changed the title
3 participants
Description
CI unit tests are failing due to setup envtest issue.
Bump controller runtime deps to use latest version of env test.
Related similar issue: metallb/metallb#2950
Summary by CodeRabbit
Bug Fixes
Chores
Tests