kubernetes/controller/internal/controller/resource/conversion/conversions_test.go (1)

1-213: LGTM! Comprehensive test coverage for CEL type conversions.

The tests thoroughly cover scalar types, collections (lists, maps), nested structures, and special types (duration, timestamp, bytes). The deep-copy test properly validates mutation isolation.

Consider adding a test for the ErrUnsupportedType error path to ensure unsupported CEL types are handled gracefully.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@kubernetes/controller/internal/controller/resource/conversion/conversions_test.go`
around lines 1 - 213, Add a new unit test in conversions_test.go that constructs
a CEL value representing an unsupported type and calls GoNativeType, asserting
it returns the package's ErrUnsupportedType (and non-nil error); specifically
reference GoNativeType and ErrUnsupportedType in the test, create or mock a cel
ref.Val that triggers the unsupported path (e.g., via
types.NewEmptyRegistry().NativeToValue with a custom/unknown native type or a
crafted cel.Value), and verify the error value equals ErrUnsupportedType and no
nil result is returned.

kubernetes/controller/examples/helm-simple-nested-status/rgd.yaml (1)

10-10: Prefer secure-by-default registry config in the example.

Line 36 hardwires insecure: true. Consider making this configurable and defaulting to false so copied manifests don’t inherit an insecure setting.

Suggested refactor

     spec:
       prefix: string | default="helm-simple-nested-status"
+      insecureRegistry: bool | default=false
@@
-          insecure: true
+          insecure: ${schema.spec.insecureRegistry}

Also applies to: 36-36

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/examples/helm-simple-nested-status/rgd.yaml` at line
10, The example hardcodes insecure: true which is unsafe; update the rgd.yaml
example to make the registry insecure flag configurable (e.g., introduce a
registry.insecure variable) and set its default to false, and update any
references that currently set insecure: true to read that variable (look for the
insecure field in the registry/remote/helm config and the prefix declaration) so
that copied manifests default to secure behavior while still allowing opt-in
insecure usage.

kubernetes/controller/internal/controller/resource/resource_controller_test.go (2)

866-870: Expand nested-object test to cover mixed literal + CEL types.

Line 866 currently builds the nested object as map[string]string, so all nested values are CEL strings. Adding at least one literal (e.g., bool/number) would validate that mixed typed nested JSON is preserved and not only string-expression leaves.

Also applies to: 886-890

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@kubernetes/controller/internal/controller/resource/resource_controller_test.go`
around lines 866 - 870, The nested test JSON currently uses map[string]string so
every nested value becomes a CEL-string; change the nested literal construction
to use map[string]interface{} (or equivalent) and set at least one nested value
to a non-string literal (e.g., boolean or number) so mixed literal +
CEL-expression types are exercised; update both occurrences that build the "oci"
Raw payload (the map passed into mustMarshalJSON in resource_controller_test.go)
and keep keys like "registry"/"repository"/"reference" but make one key a
literal (e.g., add "insecure": true or "port": 443) to validate mixed-typed
nested JSON is preserved.

---

111-127: Consider a helper for CEL JSON string encoding to reduce repetition.

Line 111–Line 127 repeats the same raw JSON-string wrapping pattern many times, which is easy to mistype and harder to maintain.

Refactor sketch

+func mustCELExprJSON(expr string) apiextensionsv1.JSON {
+	return mustToJSON(expr)
+}
...
- additionalStatusFields["registry"] = apiextensionsv1.JSON{Raw: []byte(`"resource.access.toOCI().registry"`)}
+ additionalStatusFields["registry"] = mustCELExprJSON("resource.access.toOCI().registry")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@kubernetes/controller/internal/controller/resource/resource_controller_test.go`
around lines 111 - 127, The test repeatedly constructs apiextensionsv1.JSON
values using Raw: []byte(`"..."`) (e.g., lines setting
additionalStatusFields["registry"], "repository", "reference", "helmChart",
"gitRepoURL", "gitRepository"); create a small helper function (e.g.,
celStringJSON or mustCELJSON) that accepts the unquoted CEL expression string
and returns apiextensionsv1.JSON with the correctly quoted Raw bytes, then
replace each Raw: []byte(...) site in the resource_controller_test.go setup with
calls to that helper to remove repetition and prevent quoting mistakes.

kubernetes/controller/internal/controller/resource/conversion/conversions.go (2)

116-118: Consider adding context to value conversion error.

Line 109 wraps key conversion errors with context, but value conversion errors at line 118 are returned without indicating which key caused the failure. This could make debugging harder.

♻️ Proposed fix

 		valNative, err := GoNativeType(val)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to convert map value for key %q: %w", keyStr, err)
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/controller/resource/conversion/conversions.go`
around lines 116 - 118, The value conversion error returned from GoNativeType
(when assigning valNative) lacks context about which key failed; update the
error handling around the GoNativeType call (where valNative, err :=
GoNativeType(val) is invoked) to wrap or format the error with the associated
key (e.g., include the variable key) so the returned error reads like "failed
converting value for key '<key>': <err>". Use the project's preferred
error-wrapping pattern (fmt.Errorf or errors.Wrap) and keep the variable names
(val, valNative, key, err) to locate the change.

---

102-104: Nil key handling is silent - consider logging or erroring.

CEL maps shouldn't have nil keys. If it.Next() returns nil, it may indicate an internal issue that would be silently masked here. Consider returning an error or adding a debug log for visibility.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@kubernetes/controller/internal/controller/resource/conversion/conversions.go`
around lines 102 - 104, The nil-key branch silently skips an unexpected case
from it.Next(), which masks internal errors; replace the silent continue in the
key == nil check with visible handling: either return an error (e.g.,
fmt.Errorf) from the conversion function or emit a debug/error log via your
project logger (klog/logr) including context (e.g., the iterator state and
surrounding map name). Update the branch in the function where it.Next() is
called so callers can detect and handle this abnormal nil-key instead of
silently dropping it.