Split different controllers into separate Kustomize components#651
Open
RobGeada wants to merge 7 commits intotrustyai-explainability:mainfrom
Open
Split different controllers into separate Kustomize components#651RobGeada wants to merge 7 commits intotrustyai-explainability:mainfrom
RobGeada wants to merge 7 commits intotrustyai-explainability:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
If I got this right, using kubectl apply -k config/overlays/rhoaiWon't enable any component, so one has to create a resources:
- config/overlays/rhoai
- config/components/lmes
- ...Also, I guess we are expecting to have a number of flags exposed in the DataScienceCluebt CR and then based on the flags including the appropriate components kustomization ? kind: DataScienceCluster
spec:
components:
trustyai:
components:
- lmes
- ... |
RobGeada
force-pushed
the
ControllerSeparation
branch
from
0710b5d to
cee108b
Compare
Contributor
Author
Hi @lburgazzoli, the intention here is that the overlays choose the components - so: kubectl apply -k config/overlays/rhoai will choose the relevant components for a RHOAI deployment, config/overlay/mcp-guardrails will choose the components needed for the MCP Guardrails integration, etc. So the user-facing functionality should not change, it just gives the devs more flexibility to define what should be included in an overlay. |
|
ok so it is not the intention to expose any individual component |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds componentized kustomize manifests and many component-scoped CRDs/RBAC/configmaps and overlays; introduces scripts to generate, validate, and list components and extract RBAC; updates Makefile (new targets, manifest-gen behavior) and bumps kustomize version; moves base RBAC into Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
api/gorch/v1alpha1/zz_generated.deepcopy.go (1)
19-22: Same duplicate "DO NOT EDIT" notice as in other deepcopy files.Same root cause as noted in
hack/boilerplate.go.txt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/gorch/v1alpha1/zz_generated.deepcopy.go` around lines 19 - 22, The file contains a duplicated boilerplate header ("Code generated by controller-gen. DO NOT EDIT.") at the top; remove the extra duplicate so the generated file contains a single instance of that notice, and adjust the generation step (or hack/boilerplate.go.txt used by controller-gen) so future runs emit the header only once.api/tas/v1/zz_generated.deepcopy.go (1)
19-22: Same duplicate "DO NOT EDIT" notice.Root cause flagged in
hack/boilerplate.go.txt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/tas/v1/zz_generated.deepcopy.go` around lines 19 - 22, Remove the duplicate generated header line ("// Code generated by controller-gen. DO NOT EDIT.") from zz_generated.deepcopy.go so only a single "DO NOT EDIT" notice remains, and then fix the generator boilerplate template (hack/boilerplate.go.txt) to prevent emitting the header twice on future runs; target the header string in the file and the boilerplate template to ensure the controller-gen output includes the notice only once.config/components/tas/rbac/manager-rbac.yaml (1)
187-197: Cluster-scoped RBAC write permissions (clusterrolebindings) remain — confirm necessity.Previous review flagged
create/update/deleteonclusterrolebindingsas a privilege-escalation risk. CRD permissions were reduced to read-only (good), butclusterrolebindingsstill have write access. If the TAS controller creates RBAC bindings for its managed workloads at runtime, this is justified; otherwise, consider scoping down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/components/tas/rbac/manager-rbac.yaml` around lines 187 - 197, The ClusterRoleBinding write verbs on clusterrolebindings in manager-rbac.yaml (create/update/delete) are a potential privilege-escalation risk; verify whether the TAS controller (or any component that uses the ClusterRoleBinding permissions) actually needs to create or modify cluster-scoped bindings at runtime, and if not remove the create/update/delete verbs (leaving get/list/watch) or replace the ClusterRoleBinding permission with namespace-scoped Role/RoleBinding permissions limited to the specific managed namespaces; if runtime creation is required, narrow the Scope by implementing a dedicated reconciler/service account and document why cluster-scoped write access for clusterrolebindings is necessary.hack/validate-components.sh (1)
15-24: Kustomize detection order is inverted — script may exit prematurely.Lines 16-18 exit if
kustomizeis not onPATH, before lines 21-24 ever get a chance to check${PROJECT_ROOT}/bin/kustomize. The local binary (installed viamake kustomize) is typically preferred.🔧 Proposed fix
-# Check if kustomize is available -if ! command -v kustomize &> /dev/null; then - echo "Error: kustomize not found. Please run 'make kustomize' first." - exit 1 -fi - -KUSTOMIZE="${PROJECT_ROOT}/bin/kustomize" -if [[ ! -x "${KUSTOMIZE}" ]]; then - KUSTOMIZE="kustomize" -fi +KUSTOMIZE="${PROJECT_ROOT}/bin/kustomize" +if [[ -x "${KUSTOMIZE}" ]]; then + : +elif command -v kustomize &> /dev/null; then + KUSTOMIZE="kustomize" +else + echo "Error: kustomize not found. Please run 'make kustomize' first." + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/validate-components.sh` around lines 15 - 24, The script currently exits if kustomize is not on PATH before checking the local build at ${PROJECT_ROOT}/bin/kustomize; change the detection order so the script first checks if "${PROJECT_ROOT}/bin/kustomize" is executable and assigns it to KUSTOMIZE, and only if that check fails fall back to using "command -v kustomize" (or default to "kustomize") and error out only if neither is available; update the KUSTOMIZE variable logic around the KUSTOMIZE and PROJECT_ROOT symbols and remove the premature exit that occurs before testing the local binary.hack/extract-component-rbac.sh (1)
26-27:⚠️ Potential issue | 🟡 MinorHarden temp file handling and trap quoting.
Use a portable
mktemptemplate and single-quoted trap so cleanup evaluates variables at exit time.🛠️ Proposed fix
-TEMP_RULES=$(mktemp) -trap "rm -f ${TEMP_RULES}" EXIT +TEMP_RULES="$(mktemp "${TMPDIR:-/tmp}/trustyai-component-rbac.XXXXXX")" +trap 'rm -f "$TEMP_RULES"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/extract-component-rbac.sh` around lines 26 - 27, The temp-file handling should use a portable mktemp template and a single-quoted trap so the TEMP_RULES variable is expanded at EXIT rather than when the trap is defined: change the mktemp invocation that sets TEMP_RULES to use a template (e.g., mktemp with a suffix/template) and update the trap command (the trap that removes ${TEMP_RULES}) to be single-quoted so cleanup runs correctly; modify the TEMP_RULES assignment and the trap string surrounding the existing TEMP_RULES reference (variables: TEMP_RULES, mktemp, trap) accordingly.hack/generate-components.sh (1)
71-73:⚠️ Potential issue | 🟠 MajorFail fast when a required CRD is not found.
Continuing after a missing CRD can generate incomplete components and push failure to apply/build time.
🛠️ Proposed fix
- else - echo " ✗ No CRDs for pattern '${crd_pattern}'" - fi + else + echo >&2 " ✗ Error: CRD not found for pattern '${crd_pattern}' (component: ${component})" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 71 - 73, The script currently prints "✗ No CRDs for pattern '${crd_pattern}'" and continues, which can lead to incomplete generated components; update the else branch where crd_pattern is checked (the block that echoes the missing CRD) to fail fast by printing the same diagnostic and then exiting with a non-zero status (e.g., exit 1) so the generator stops immediately when a required CRD pattern is not found.hack/kubebuilder-rbac-to-yaml.sh (2)
66-104:⚠️ Potential issue | 🟠 MajorDo not emit
apiGroupsforurls=-only RBAC markers.Current output always starts with
apiGroups, which can produce invalid RBAC rules when a marker defines onlyurls(non-resource URL rule).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/kubebuilder-rbac-to-yaml.sh` around lines 66 - 104, The script always emits the top-level "- apiGroups:" header (the echo "- apiGroups:" line) which creates invalid RBAC when a marker defines only nonResourceURLs (urls); change the logic so the apiGroups block is only emitted when appropriate (e.g., when groups is non-empty or when this is not a urls-only rule). Concretely, wrap or gate the existing apiGroups output and the subsequent groups handling behind a conditional that checks the presence of groups/resources/verbs (for example if [[ -n "${groups}" || -n "${resources}" || -n "${verbs}" ]]) so that when only urls is provided the script skips printing "- apiGroups:" and only emits the nonResourceURLs section.
13-15:⚠️ Potential issue | 🟡 MinorUse portable temp-file creation and single-quoted trap cleanup.
mktempwithout a template is not portable, and the double-quoted trap expands variables too early.🛠️ Proposed fix
-TEMP_FILE=$(mktemp) -TEMP_SEEN=$(mktemp) -trap "rm -f ${TEMP_FILE} ${TEMP_SEEN}" EXIT +TEMP_FILE="$(mktemp "${TMPDIR:-/tmp}/trustyai-rbac.XXXXXX")" +TEMP_SEEN="$(mktemp "${TMPDIR:-/tmp}/trustyai-rbac-seen.XXXXXX")" +trap 'rm -f "$TEMP_FILE" "$TEMP_SEEN"' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/kubebuilder-rbac-to-yaml.sh` around lines 13 - 15, Replace non-portable mktemp calls and the double-quoted trap: create TEMP_FILE and TEMP_SEEN with a template-aware mktemp (e.g., mktemp "${TMPDIR:-/tmp}/kubebuilder-rbac.XXXXXX" or similar template) instead of bare mktemp, and change the trap line to use single quotes so variables expand at trap execution: use trap 'rm -f "$TEMP_FILE" "$TEMP_SEEN"' EXIT; update the lines that set TEMP_FILE, TEMP_SEEN and the trap accordingly (referencing TEMP_FILE, TEMP_SEEN, and the trap command).Makefile (1)
302-329:⚠️ Potential issue | 🟠 MajorFix
manifest-genoverlay flow (basehandling + default consistency).
OVERLAY=basebypasses validation but then triesconfig/overlays/base(invalid path), and unsetOVERLAYbuildstestingwhile help text saysbase (default).🛠️ Proposed fix
- echo " base (default)"; \ + echo " odh (default)"; \ @@ - `@if` [ -z "$(OVERLAY)" ]; then \ - echo "Building from config/overlays/testing..."; \ - $(KUSTOMIZE) build config/overlays/testing | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_bundle.yaml; \ + `@if` [ -z "$(OVERLAY)" ]; then \ + echo "Building from config/overlays/odh..."; \ + $(KUSTOMIZE) build config/overlays/odh | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_bundle.yaml; \ echo "✓ Release manifest generated at release/trustyai_bundle.yaml"; \ + elif [ "$(OVERLAY)" = "base" ]; then \ + echo "Building from config/base..."; \ + $(KUSTOMIZE) build config/base | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_base_bundle.yaml; \ + echo "✓ Release manifest generated at release/trustyai_base_bundle.yaml"; \ else \ echo "Building from config/overlays/$(OVERLAY)..."; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 302 - 329, The manifest-gen target mishandles the "base" default: when OVERLAY is unset it builds config/overlays/testing (contradicting help text), and when OVERLAY=base it bypasses validation but later attempts config/overlays/base which may be wrong. Fix by treating "base" as the default overlay: set OVERLAY?=base (or if [ -z "$(OVERLAY)" ] then OVERLAY=base) before validation, change the build branch that currently uses config/overlays/testing to use config/overlays/base, and simplify validation to only error if OVERLAY is non-empty and the directory config/overlays/$(OVERLAY) does not exist; also ensure the output filename and messages use $(OVERLAY) consistently (e.g., release/trustyai_$(OVERLAY)_bundle.yaml) and that OPERATOR_IMAGE substitution still applies.
🧹 Nitpick comments (4)
api/evalhub/v1alpha1/zz_generated.deepcopy.go (1)
19-22: Duplicate "DO NOT EDIT" notice — root cause is inhack/boilerplate.go.txt.Lines 19 and 21 both contain
// Code generated by controller-gen. DO NOT EDIT.— one injected by the boilerplate template and one by controller-gen itself. This is a cosmetic issue; see the comment onhack/boilerplate.go.txtfor the root-cause fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/evalhub/v1alpha1/zz_generated.deepcopy.go` around lines 19 - 22, The generated file zz_generated.deepcopy.go contains the duplicate header comment "// Code generated by controller-gen. DO NOT EDIT." because the boilerplate template used during generation also injects the same line; open the boilerplate template used by controller-gen and remove or dedupe that specific line so only controller-gen emits the header (rebuild the generated file afterward); target the boilerplate template entry that inserts the "// Code generated by controller-gen. DO NOT EDIT." string and ensure the generator’s own header remains singular to eliminate the duplicate in zz_generated.deepcopy.go.config/base-rbac/kustomization.yaml (1)
1-9: Inconsistent file extension:auth-delegator.ymluses.ymlwhile all other resources use.yaml.Not a functional issue (kustomize handles both extensions), but worth standardizing for consistency. Consider renaming to
auth-delegator.yamlto match the convention used by the remaining 7 resources in this directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/base-rbac/kustomization.yaml` around lines 1 - 9, Update the resource entry in the kustomization resources list to use a consistent .yaml extension by changing the reference from auth-delegator.yml to auth-delegator.yaml and rename the actual file in the repo to auth-delegator.yaml so the resources block (the resources: list in config/base-rbac/kustomization.yaml) matches the other entries.hack/validate-components.sh (2)
96-98: Empty array expansion withset -umay fail on older Bash.If no components validate (or none fail), iterating
"${VALIDATED_COMPONENTS[@]}"withset -ucauses an "unbound variable" error in Bash < 4.4. The same applies toFAILED_COMPONENTS,VALIDATED_OVERLAYS, andFAILED_OVERLAYS. Guard with${arr[@]+"${arr[@]}"}or check length first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/validate-components.sh` around lines 96 - 98, Loops and echo lines that expand arrays like VALIDATED_COMPONENTS, FAILED_COMPONENTS, VALIDATED_OVERLAYS, and FAILED_OVERLAYS can break under set -u when the arrays are empty; update each use (the echo of counts and each for ... in "${ARRAY[@]}" loop such as the "echo \"Validated components: ${`#VALIDATED_COMPONENTS`[@]}\"" and the for loop over VALIDATED_COMPONENTS) to guard the expansion either by checking the length first (if [ "${`#VALIDATED_COMPONENTS`[@]}" -gt 0 ]; then ... ) or by using the safe expansion pattern ${ARRAY[@]+"${ARRAY[@]}"} so empty arrays don't trigger unbound variable errors in older Bash versions.
55-60: Component is built twice — capture output in a single invocation.Line 56 builds to
/dev/nullto test success, then line 60 builds again to capture output. You can do both in one pass.♻️ Proposed fix
- if "${KUSTOMIZE}" build "${component_dir}" > /dev/null 2>&1; then - echo " ✓ Component builds successfully" - - # Check that output contains expected resources - output=$("${KUSTOMIZE}" build "${component_dir}") + if output=$("${KUSTOMIZE}" build "${component_dir}" 2>&1); then + echo " ✓ Component builds successfully"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/validate-components.sh` around lines 55 - 60, The script currently runs "${KUSTOMIZE} build ${component_dir}" twice (once redirecting to /dev/null to test success and again to capture output in variable output); change this to a single invocation: run the build once, capture both stdout/stderr into a variable (e.g., output=$("${KUSTOMIZE}" build "${component_dir}" 2>&1) or use if output=$("${KUSTOMIZE}" build "${component_dir}" 2>&1); then check the command exit status (if or $? ) to decide success and use the captured output variable for the subsequent resource checks, updating references to output and the success/echo logic accordingly.
🤖 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/components/evalhub/rbac/manager-rolebinding.yaml`:
- Line 15: Change the hardcoded prefixed value in roleRef.name to the short
ClusterRole name so kustomize can rewrite it; specifically replace roleRef.name:
trustyai-service-operator-evalhub-manager-role with roleRef.name:
evalhub-manager-role to match the ClusterRole named evalhub-manager-role (in the
manager RoleBinding/ClusterRoleBinding resource) and allow the namePrefix
transformer to apply the trustyai-service-operator- prefix automatically.
In `@config/components/gorch/rbac/manager-rbac.yaml`:
- Around line 86-97: The pods rule currently grants full CRUD verbs (create,
update, patch, delete) but the gorch controller never mutates pods; update the
pods RBAC rule (apiGroups: "" resources: ["pods"]) to only include read verbs
("get", "list", "watch") — remove "create", "update", "patch", and "delete" from
the verbs list so the controller has read-only pod permissions.
In `@config/components/gorch/rbac/manager-rolebinding.yaml`:
- Around line 12-15: Update the roleRef.name in the manager RoleBinding so it
references the short ClusterRole name instead of the already-prefixed name:
change roleRef.name in manager-rolebinding.yaml from
"trustyai-service-operator-gorch-manager-role" to "gorch-manager-role" (keep
apiGroup and kind unchanged) so kustomize namePrefix can correctly transform and
match the ClusterRole; apply the same fix pattern to other component
rolebindings (evalhub, job-mgr, lmes, nemo-guardrails, tas) where roleRef.name
is hardcoded with the prefix.
In `@hack/boilerplate.go.txt`:
- Line 17: Remove the duplicate generated notice from the boilerplate template:
open the boilerplate.go.txt used as the --go-header-file and delete the
standalone line "// Code generated by controller-gen. DO NOT EDIT." so
controller-gen can insert its own generated notice. Ensure the rest of the
header remains intact; verify generated files (e.g., zz_generated.deepcopy.go)
no longer contain two "// Code generated by controller-gen. DO NOT EDIT." lines.
In `@hack/generate-components.sh`:
- Around line 61-70: The script moves CRD files into "${component_dir}/crd" but
never creates that directory, causing mv to fail; before the mv call in the
block handling crd_pattern (the section using crd_file, CRD_BASES_DIR, and the
mv -f "${crd_file}" "${component_dir}/crd/$(basename "${crd_file}")"), ensure
the target directory exists by creating "${component_dir}/crd" (e.g., mkdir -p
"${component_dir}/crd") right after creating "${component_dir}/rbac" and before
the find/mv logic so the mv cannot fail due to a missing directory.
- Line 101: The ClusterRoleBinding's roleRef.name is using the full prefixed
string ("trustyai-service-operator-${component}-manager-role") while the
generated ClusterRole uses the short name pattern ("${component}-manager-role");
update the roleRef.name to use the same short name pattern as the ClusterRole
(i.e., "${component}-manager-role" or the existing short-name variable) so
kustomize namePrefix can be applied consistently; modify the roleRef.name value
in the manifest generation logic that emits roleRef to match the ClusterRole
name generation (referencing the ClusterRole name generation symbol/pattern and
roleRef.name).
---
Duplicate comments:
In `@api/gorch/v1alpha1/zz_generated.deepcopy.go`:
- Around line 19-22: The file contains a duplicated boilerplate header ("Code
generated by controller-gen. DO NOT EDIT.") at the top; remove the extra
duplicate so the generated file contains a single instance of that notice, and
adjust the generation step (or hack/boilerplate.go.txt used by controller-gen)
so future runs emit the header only once.
In `@api/tas/v1/zz_generated.deepcopy.go`:
- Around line 19-22: Remove the duplicate generated header line ("// Code
generated by controller-gen. DO NOT EDIT.") from zz_generated.deepcopy.go so
only a single "DO NOT EDIT" notice remains, and then fix the generator
boilerplate template (hack/boilerplate.go.txt) to prevent emitting the header
twice on future runs; target the header string in the file and the boilerplate
template to ensure the controller-gen output includes the notice only once.
In `@config/components/tas/rbac/manager-rbac.yaml`:
- Around line 187-197: The ClusterRoleBinding write verbs on clusterrolebindings
in manager-rbac.yaml (create/update/delete) are a potential privilege-escalation
risk; verify whether the TAS controller (or any component that uses the
ClusterRoleBinding permissions) actually needs to create or modify
cluster-scoped bindings at runtime, and if not remove the create/update/delete
verbs (leaving get/list/watch) or replace the ClusterRoleBinding permission with
namespace-scoped Role/RoleBinding permissions limited to the specific managed
namespaces; if runtime creation is required, narrow the Scope by implementing a
dedicated reconciler/service account and document why cluster-scoped write
access for clusterrolebindings is necessary.
In `@hack/extract-component-rbac.sh`:
- Around line 26-27: The temp-file handling should use a portable mktemp
template and a single-quoted trap so the TEMP_RULES variable is expanded at EXIT
rather than when the trap is defined: change the mktemp invocation that sets
TEMP_RULES to use a template (e.g., mktemp with a suffix/template) and update
the trap command (the trap that removes ${TEMP_RULES}) to be single-quoted so
cleanup runs correctly; modify the TEMP_RULES assignment and the trap string
surrounding the existing TEMP_RULES reference (variables: TEMP_RULES, mktemp,
trap) accordingly.
In `@hack/generate-components.sh`:
- Around line 71-73: The script currently prints "✗ No CRDs for pattern
'${crd_pattern}'" and continues, which can lead to incomplete generated
components; update the else branch where crd_pattern is checked (the block that
echoes the missing CRD) to fail fast by printing the same diagnostic and then
exiting with a non-zero status (e.g., exit 1) so the generator stops immediately
when a required CRD pattern is not found.
In `@hack/kubebuilder-rbac-to-yaml.sh`:
- Around line 66-104: The script always emits the top-level "- apiGroups:"
header (the echo "- apiGroups:" line) which creates invalid RBAC when a marker
defines only nonResourceURLs (urls); change the logic so the apiGroups block is
only emitted when appropriate (e.g., when groups is non-empty or when this is
not a urls-only rule). Concretely, wrap or gate the existing apiGroups output
and the subsequent groups handling behind a conditional that checks the presence
of groups/resources/verbs (for example if [[ -n "${groups}" || -n "${resources}"
|| -n "${verbs}" ]]) so that when only urls is provided the script skips
printing "- apiGroups:" and only emits the nonResourceURLs section.
- Around line 13-15: Replace non-portable mktemp calls and the double-quoted
trap: create TEMP_FILE and TEMP_SEEN with a template-aware mktemp (e.g., mktemp
"${TMPDIR:-/tmp}/kubebuilder-rbac.XXXXXX" or similar template) instead of bare
mktemp, and change the trap line to use single quotes so variables expand at
trap execution: use trap 'rm -f "$TEMP_FILE" "$TEMP_SEEN"' EXIT; update the
lines that set TEMP_FILE, TEMP_SEEN and the trap accordingly (referencing
TEMP_FILE, TEMP_SEEN, and the trap command).
In `@hack/validate-components.sh`:
- Around line 15-24: The script currently exits if kustomize is not on PATH
before checking the local build at ${PROJECT_ROOT}/bin/kustomize; change the
detection order so the script first checks if "${PROJECT_ROOT}/bin/kustomize" is
executable and assigns it to KUSTOMIZE, and only if that check fails fall back
to using "command -v kustomize" (or default to "kustomize") and error out only
if neither is available; update the KUSTOMIZE variable logic around the
KUSTOMIZE and PROJECT_ROOT symbols and remove the premature exit that occurs
before testing the local binary.
In `@Makefile`:
- Around line 302-329: The manifest-gen target mishandles the "base" default:
when OVERLAY is unset it builds config/overlays/testing (contradicting help
text), and when OVERLAY=base it bypasses validation but later attempts
config/overlays/base which may be wrong. Fix by treating "base" as the default
overlay: set OVERLAY?=base (or if [ -z "$(OVERLAY)" ] then OVERLAY=base) before
validation, change the build branch that currently uses config/overlays/testing
to use config/overlays/base, and simplify validation to only error if OVERLAY is
non-empty and the directory config/overlays/$(OVERLAY) does not exist; also
ensure the output filename and messages use $(OVERLAY) consistently (e.g.,
release/trustyai_$(OVERLAY)_bundle.yaml) and that OPERATOR_IMAGE substitution
still applies.
---
Nitpick comments:
In `@api/evalhub/v1alpha1/zz_generated.deepcopy.go`:
- Around line 19-22: The generated file zz_generated.deepcopy.go contains the
duplicate header comment "// Code generated by controller-gen. DO NOT EDIT."
because the boilerplate template used during generation also injects the same
line; open the boilerplate template used by controller-gen and remove or dedupe
that specific line so only controller-gen emits the header (rebuild the
generated file afterward); target the boilerplate template entry that inserts
the "// Code generated by controller-gen. DO NOT EDIT." string and ensure the
generator’s own header remains singular to eliminate the duplicate in
zz_generated.deepcopy.go.
In `@config/base-rbac/kustomization.yaml`:
- Around line 1-9: Update the resource entry in the kustomization resources list
to use a consistent .yaml extension by changing the reference from
auth-delegator.yml to auth-delegator.yaml and rename the actual file in the repo
to auth-delegator.yaml so the resources block (the resources: list in
config/base-rbac/kustomization.yaml) matches the other entries.
In `@hack/validate-components.sh`:
- Around line 96-98: Loops and echo lines that expand arrays like
VALIDATED_COMPONENTS, FAILED_COMPONENTS, VALIDATED_OVERLAYS, and FAILED_OVERLAYS
can break under set -u when the arrays are empty; update each use (the echo of
counts and each for ... in "${ARRAY[@]}" loop such as the "echo \"Validated
components: ${`#VALIDATED_COMPONENTS`[@]}\"" and the for loop over
VALIDATED_COMPONENTS) to guard the expansion either by checking the length first
(if [ "${`#VALIDATED_COMPONENTS`[@]}" -gt 0 ]; then ... ) or by using the safe
expansion pattern ${ARRAY[@]+"${ARRAY[@]}"} so empty arrays don't trigger
unbound variable errors in older Bash versions.
- Around line 55-60: The script currently runs "${KUSTOMIZE} build
${component_dir}" twice (once redirecting to /dev/null to test success and again
to capture output in variable output); change this to a single invocation: run
the build once, capture both stdout/stderr into a variable (e.g.,
output=$("${KUSTOMIZE}" build "${component_dir}" 2>&1) or use if
output=$("${KUSTOMIZE}" build "${component_dir}" 2>&1); then check the command
exit status (if or $? ) to decide success and use the captured output variable
for the subsequent resource checks, updating references to output and the
success/echo logic accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
Makefileapi/evalhub/v1alpha1/zz_generated.deepcopy.goapi/gorch/v1alpha1/zz_generated.deepcopy.goapi/lmes/v1alpha1/zz_generated.deepcopy.goapi/nemo_guardrails/v1alpha1/zz_generated.deepcopy.goapi/tas/v1/zz_generated.deepcopy.goapi/tas/v1alpha1/zz_generated.deepcopy.goconfig/base-rbac/auth-delegator.ymlconfig/base-rbac/auth_proxy_client_clusterrole.yamlconfig/base-rbac/auth_proxy_role.yamlconfig/base-rbac/auth_proxy_role_binding.yamlconfig/base-rbac/auth_proxy_service.yamlconfig/base-rbac/kustomization.yamlconfig/base-rbac/leader_election_role.yamlconfig/base-rbac/leader_election_role_binding.yamlconfig/base-rbac/service_account.yamlconfig/base/kustomization.yamlconfig/components/evalhub/crd/trustyai.opendatahub.io_evalhubs.yamlconfig/components/evalhub/kustomization.yamlconfig/components/evalhub/rbac/evalhub_auth_reviewer_role.yamlconfig/components/evalhub/rbac/evalhub_job_config_binding.yamlconfig/components/evalhub/rbac/evalhub_job_config_role.yamlconfig/components/evalhub/rbac/evalhub_jobs_writer_binding.yamlconfig/components/evalhub/rbac/evalhub_jobs_writer_role.yamlconfig/components/evalhub/rbac/evalhub_mlflow_access_binding.yamlconfig/components/evalhub/rbac/evalhub_mlflow_access_role.yamlconfig/components/evalhub/rbac/evalhub_mlflow_jobs_binding.yamlconfig/components/evalhub/rbac/evalhub_mlflow_jobs_role.yamlconfig/components/evalhub/rbac/manager-rbac.yamlconfig/components/evalhub/rbac/manager-rolebinding.yamlconfig/components/gorch/crd/trustyai.opendatahub.io_guardrailsorchestrators.yamlconfig/components/gorch/kustomization.yamlconfig/components/gorch/rbac/manager-rbac.yamlconfig/components/gorch/rbac/manager-rolebinding.yamlconfig/components/job-mgr/kustomization.yamlconfig/components/job-mgr/rbac/manager-rbac.yamlconfig/components/job-mgr/rbac/manager-rolebinding.yamlconfig/components/lmes/configmaps/lmeval-tasks.yamlconfig/components/lmes/crd/trustyai.opendatahub.io_lmevaljobs.yamlconfig/components/lmes/kustomization.yamlconfig/components/lmes/rbac/default_lmeval_user_rolebinding.yamlconfig/components/lmes/rbac/manager-rbac.yamlconfig/components/lmes/rbac/manager-rolebinding.yamlconfig/components/lmes/rbac/non_admin_lmeval_role.yamlconfig/components/nemo-guardrails/crd/trustyai.opendatahub.io_nemoguardrails.yamlconfig/components/nemo-guardrails/kustomization.yamlconfig/components/nemo-guardrails/rbac/manager-rbac.yamlconfig/components/nemo-guardrails/rbac/manager-rolebinding.yamlconfig/components/nemo-guardrails/rbac/nemoguardrail_editor_role.yamlconfig/components/nemo-guardrails/rbac/nemoguardrail_viewer_role.yamlconfig/components/tas/crd/trustyai.opendatahub.io_trustyaiservices.yamlconfig/components/tas/kustomization.yamlconfig/components/tas/rbac/manager-rbac.yamlconfig/components/tas/rbac/manager-rolebinding.yamlconfig/components/tas/rbac/trustyaiservice_editor_role.yamlconfig/components/tas/rbac/trustyaiservice_viewer_role.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/overlays/lmes/kustomization.yamlconfig/overlays/mcp-guardrails/kustomization.yamlconfig/overlays/mcp-guardrails/params.envconfig/overlays/odh-kueue/kustomization.yamlconfig/overlays/odh/kustomization.yamlconfig/overlays/rhoai/kustomization.yamlconfig/overlays/testing/kustomization.yamlconfig/rbac/kustomization.yamlhack/boilerplate.go.txthack/boilerplate.yaml.txthack/extract-component-rbac.shhack/generate-component-kustomization.shhack/generate-components.shhack/kubebuilder-rbac-to-yaml.shhack/validate-components.sh
💤 Files with no reviewable changes (1)
- config/rbac/kustomization.yaml
✅ Files skipped from review due to trivial changes (9)
- api/tas/v1alpha1/zz_generated.deepcopy.go
- config/components/lmes/crd/trustyai.opendatahub.io_lmevaljobs.yaml
- hack/boilerplate.yaml.txt
- config/components/tas/crd/trustyai.opendatahub.io_trustyaiservices.yaml
- api/nemo_guardrails/v1alpha1/zz_generated.deepcopy.go
- config/components/gorch/crd/trustyai.opendatahub.io_guardrailsorchestrators.yaml
- api/lmes/v1alpha1/zz_generated.deepcopy.go
- config/components/evalhub/crd/trustyai.opendatahub.io_evalhubs.yaml
- config/components/nemo-guardrails/crd/trustyai.opendatahub.io_nemoguardrails.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- config/components/tas/kustomization.yaml
- config/components/tas/rbac/manager-rolebinding.yaml
- config/manager/manager.yaml
- config/components/lmes/kustomization.yaml
- config/components/lmes/rbac/manager-rolebinding.yaml
- config/components/evalhub/kustomization.yaml
- hack/generate-component-kustomization.sh
- config/components/job-mgr/rbac/manager-rolebinding.yaml
- config/components/nemo-guardrails/rbac/manager-rolebinding.yaml
- config/components/lmes/configmaps/lmeval-tasks.yaml
- config/components/nemo-guardrails/kustomization.yaml
- config/components/lmes/rbac/manager-rbac.yaml
- config/overlays/odh/kustomization.yaml
- config/components/evalhub/rbac/manager-rbac.yaml
- config/components/gorch/kustomization.yaml
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: manager-role | ||
| name: trustyai-service-operator-evalhub-manager-role |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check namePrefix and other settings in the evalhub component kustomization
echo "=== config/components/evalhub/kustomization.yaml ==="
if [ -f "config/components/evalhub/kustomization.yaml" ]; then
cat -n config/components/evalhub/kustomization.yaml
else
echo "File not found"
fi
echo -e "\n=== ClusterRole name in manager-rbac.yaml ==="
if [ -f "config/components/evalhub/rbac/manager-rbac.yaml" ]; then
cat -n config/components/evalhub/rbac/manager-rbac.yaml
else
echo "File not found"
fi
echo -e "\n=== RoleBinding file for reference ==="
if [ -f "config/components/evalhub/rbac/manager-rolebinding.yaml" ]; then
cat -n config/components/evalhub/rbac/manager-rolebinding.yaml
else
echo "File not found"
fiRepository: trustyai-explainability/trustyai-service-operator
Length of output: 4497
roleRef.name should use the short name to align with kustomize namePrefix pattern.
The ClusterRole in manager-rbac.yaml is correctly defined with the short name evalhub-manager-role. The roleRef.name at line 15 should match this short name so that kustomize's nameReference transformer can automatically rewrite it to the fully-qualified name when namePrefix: trustyai-service-operator- is applied. Hardcoding the already-prefixed value trustyai-service-operator-evalhub-manager-role breaks this transformation and creates a mismatch between the role and its reference.
Proposed fix
- name: trustyai-service-operator-evalhub-manager-role
+ name: evalhub-manager-role📝 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.
Suggested change
| name: trustyai-service-operator-evalhub-manager-role | |
| name: evalhub-manager-role |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/components/evalhub/rbac/manager-rolebinding.yaml` at line 15, Change
the hardcoded prefixed value in roleRef.name to the short ClusterRole name so
kustomize can rewrite it; specifically replace roleRef.name:
trustyai-service-operator-evalhub-manager-role with roleRef.name:
evalhub-manager-role to match the ClusterRole named evalhub-manager-role (in the
manager RoleBinding/ClusterRoleBinding resource) and allow the namePrefix
transformer to apply the trustyai-service-operator- prefix automatically.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the specific discussion comment that was referenced
gh api /repos/trustyai-explainability/trustyai-service-operator/pulls/651/comments/2858807923 --jq '{body: .body, user: .user.login, path: .path, line: .line}'
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Fetch review comments from PR 651 and look for discussions about naming/roleRef
gh api "/repos/trustyai-explainability/trustyai-service-operator/pulls/651/comments" \
--paginate \
--jq '.[] | select(.body | contains("roleRef") or contains("namePrefix") or contains("short name")) | {id: .id, user: .user.login, path: .path, line: .line, body: .body}' | head -20
echo "---"
echo "Also checking for comments around evalhub naming:"
gh api "/repos/trustyai-explainability/trustyai-service-operator/pulls/651/comments" \
--paginate \
--jq '.[] | select(.path | contains("evalhub")) | select(.body | contains("name")) | {id: .id, user: .user.login, path: .path, body: .body[:200]}' | head -20
Length of output: 30146
@RobGeada, thanks for pointing me to that discussion! I see the context now. In the gorch component thread, you explained that using short names in roleRef broke the ClusterRoleBindings because kustomize couldn't properly correlate them with the fully-qualified names created on the cluster, and that the hardcoded full prefix approach was the only working solution.
That same pattern applies here in the evalhub component (and across all the other components: lmes, tas, nemo-guardrails, job-mgr). The hardcoded trustyai-service-operator-{component}-manager-role names in the roleRef fields are intentional and necessary based on your testing.
I should note there's a learning in my context that suggests short names should be used with kustomize's namePrefix, but your actual experience shows that doesn't work without additional NameReference transformer configuration (which isn't currently in the repo).
Would you like me to update that learning to reflect that in this repository, the roleRef names in component rolebindings intentionally use the full prefixed form because the short-name approach didn't work in practice?
🧠 Learnings used
Learnt from: scheruku-rh
Repo: trustyai-explainability/trustyai-service-operator PR: 652
File: config/rbac/evalhub/evalhub_secrets_reader_role.yaml:1-13
Timestamp: 2026-02-25T12:57:09.782Z
Learning: In the trustyai-service-operator repository, RBAC resources in config/rbac/ are deployed through config/base/kustomization.yaml which applies namePrefix: trustyai-service-operator-. ClusterRole names in YAML manifests should use short names (e.g., evalhub-secrets-reader) as kustomize automatically prefixes them at deployment time to match Go constants (e.g., trustyai-service-operator-evalhub-secrets-reader).
Learnt from: tarilabs
Repo: trustyai-explainability/trustyai-service-operator PR: 650
File: config/configmaps/evalhub/provider-garak.yaml:24-26
Timestamp: 2026-02-18T18:07:47.424Z
Learning: In the trustyai-service-operator EvalHub provider ConfigMaps (config/configmaps/evalhub/provider-*.yaml), the provider configurations are sourced from the upstream eval-hub repository via hack/sync-evalhub-providers.py script. The project policy is to preserve the upstream provider configs as-is, including any placeholder environment variables or template content, to maintain consistency with the upstream source.
Comment on lines
+12
to
+15
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: trustyai-service-operator-gorch-manager-role |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1. Find and check the actual ClusterRole name in gorch manager-rbac.yaml
echo "=== ClusterRole definition in gorch manager-rbac.yaml ==="
find config/components -name "manager-rbac.yaml" -path "*/gorch/*" -exec cat {} \;
echo ""
# 2. Check the rolebinding file in question
echo "=== Current rolebinding file ==="
cat config/components/gorch/rbac/manager-rolebinding.yaml
echo ""
# 3. Compare with other component rolebindings
echo "=== Other component rolebinding roleRef patterns ==="
find config/components -name "*rolebinding.yaml" -type f -exec sh -c 'echo "--- {} ---"; grep -A3 "roleRef:" "$1"' _ {} \;
echo ""
# 4. Check if config/rbac/ exists and how it structures names
echo "=== Structure of config/rbac/ if it exists ==="
if [ -d "config/rbac" ]; then
find config/rbac -name "*rolebinding.yaml" -type f | head -5 | xargs -I {} sh -c 'echo "--- {} ---"; cat {}'
fiRepository: trustyai-explainability/trustyai-service-operator
Length of output: 4001
🏁 Script executed:
# Check config/rbac/ structure and rolebinding patterns
echo "=== Config/rbac rolebinding examples ==="
find config/rbac -name "*rolebinding.yaml" -type f | head -3 | while read file; do
echo "--- $file ---"
cat "$file"
echo ""
done
echo "=== Kustomization files with namePrefix ==="
find config -name "kustomization.yaml" -type f | xargs grep -l "namePrefix" | head -5
echo ""
echo "=== Check namePrefix in key kustomization files ==="
for file in config/base/kustomization.yaml config/overlays/*/kustomization.yaml; do
if [ -f "$file" ]; then
echo "--- $file ---"
grep "namePrefix" "$file" || echo "No namePrefix found"
fi
done
echo ""
echo "=== Check if components kustomization includes config/rbac ==="
grep -r "config/rbac" config/components --include="*.yaml" 2>/dev/null || echo "No direct reference to config/rbac in components"
echo ""
echo "=== Check component kustomization.yaml structure ==="
cat config/components/gorch/kustomization.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 1474
🏁 Script executed:
# Check for any build outputs, tests, or Go constants that reference these names
echo "=== Search for Go constants related to RBAC names ==="
rg -i "gorch.*manager.*role" --type go | head -10
echo ""
echo "=== Look for test files or deployment examples ==="
find . -name "*test*" -o -name "*example*" | grep -i rbac | head -5
echo ""
echo "=== Check if there's a Makefile or build script that shows deployed names ==="
find . -maxdepth 2 -name "Makefile" -o -name "*.sh" | head -3 | xargs grep -l "kustomize\|kubectl" 2>/dev/null || echo "No Makefile found"
echo ""
echo "=== Verify the exact roleRef pattern in evalhub (from learning) ==="
echo "--- evalhub manager-rbac.yaml ---"
cat config/components/evalhub/rbac/manager-rbac.yaml 2>/dev/null | grep -A1 "^ name:"
echo ""
echo "--- evalhub manager-rolebinding.yaml ---"
grep "name:" config/components/evalhub/rbac/manager-rolebinding.yaml
echo ""
echo "=== Check evalhub secrets reader role (mentioned in learning) ==="
if [ -f "config/rbac/evalhub/evalhub_secrets_reader_role.yaml" ]; then
cat config/rbac/evalhub/evalhub_secrets_reader_role.yaml
else
find . -name "*evalhub*secrets*reader*" -type f | head -3
fiRepository: trustyai-explainability/trustyai-service-operator
Length of output: 881
🏁 Script executed:
# Check the RBAC tests to see what names they expect
echo "=== Check evalhub RBAC tests ==="
head -100 ./controllers/evalhub/rbac_manifests_test.go
echo ""
echo "=== Check Makefile for kustomize build commands ==="
grep -A5 "kustomize" ./Makefile | head -20
echo ""
echo "=== Check the generate script ==="
head -50 ./hack/generate-component-kustomization.sh
echo ""
echo "=== Search for any documentation about RBAC naming ==="
find . -name "README*" -o -name "*.md" | xargs grep -l "RBAC\|kustomize" 2>/dev/null | head -3Repository: trustyai-explainability/trustyai-service-operator
Length of output: 4175
🏁 Script executed:
# Find config/rbac structure
echo "=== Search for config/rbac directory ==="
find . -type d -name "rbac" | grep -E "config/rbac|config/.*/rbac"
echo ""
echo "=== List all directories under config/ ==="
ls -la config/ 2>/dev/null || echo "No config directory at root"
echo ""
echo "=== Check if config/rbac exists with different approach ==="
if [ -d "config/rbac" ]; then
echo "config/rbac EXISTS:"
find config/rbac -type f -name "*.yaml" | head -10
else
echo "config/rbac does NOT exist"
fi
echo ""
echo "=== Look at config/default or overlays to see what they include ==="
echo "--- config/default/kustomization.yaml (if exists) ---"
if [ -f "config/default/kustomization.yaml" ]; then
cat config/default/kustomization.yaml
fi
echo ""
echo "=== Check how overlays reference RBAC ==="
echo "--- config/overlays/odh/kustomization.yaml ---"
cat config/overlays/odh/kustomization.yaml 2>/dev/null | head -40Repository: trustyai-explainability/trustyai-service-operator
Length of output: 2274
🏁 Script executed:
# Check config/base-rbac structure to understand the intended pattern
echo "=== Content of config/base-rbac ==="
find config/base-rbac -name "*.yaml" -type f | head -10 | while read file; do
echo "--- $file ---"
grep -E "^(metadata| name:| kind:)" "$file" | head -15
done
echo ""
echo "=== Check if base-rbac has rolebindings ==="
find config/base-rbac -name "*rolebinding*" -type f | while read file; do
echo "--- $file ---"
cat "$file"
done
echo ""
echo "=== Check config/base kustomization to understand setup ==="
cat config/base/kustomization.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 1987
🏁 Script executed:
# Verify if the hardcoded prefix pattern actually works or causes issues
echo "=== Check if there are any build/test outputs showing actual deployed names ==="
find . -name "*.log" -o -name "*build*" -type d | head -5
echo ""
echo "=== Look for any CI/CD validation scripts ==="
cat ./hack/validate-components.sh 2>/dev/null || echo "No validate script"
echo ""
echo "=== Check if there are examples of deployed manifests ==="
ls -la config/manifests/ 2>/dev/null || echo "No manifests directory"
echo ""
echo "=== Verify the exact kustomize behavior: does the prefix get applied correctly? ==="
echo "Let's check if there's any indication of how names should be structured in the codebase comments"
rg -i "nameprefix\|kustomize.*prefix" --type yaml -B2 -A2 config/
echo ""
echo "=== Check all component rolebinding names for consistency ==="
find config/components -name "manager-rolebinding.yaml" -type f -exec sh -c 'echo "--- {} ---"; grep "name:" {} | grep -v "app.kubernetes.io"' \;Repository: trustyai-explainability/trustyai-service-operator
Length of output: 6473
🏁 Script executed:
# Build an overlay with namePrefix to see what the actual final names are
echo "=== Build odh overlay to see final resource names ==="
cd config/overlays/odh
kustomize build . 2>/dev/null | grep -A10 "kind: ClusterRole" | grep -E "kind:|name:" | head -20
echo ""
kustomize build . 2>/dev/null | grep -A10 "kind: ClusterRoleBinding" | grep -B2 "kind: ClusterRole" | grep -E "kind:|name:" | head -40
cd ../../..
echo ""
echo "=== Specifically check gorch component roleRef after namePrefix is applied ==="
cd config/overlays/odh
kustomize build . 2>/dev/null | grep -B5 -A8 "gorch-manager-role"
cd ../../..Repository: trustyai-explainability/trustyai-service-operator
Length of output: 227
🏁 Script executed:
# Check if kustomize is available and working
echo "=== Check kustomize availability ==="
which kustomize || echo "kustomize not in PATH"
echo ""
# Try to build with explicit error output
echo "=== Try building overlay with verbose output ==="
cd config/overlays/odh 2>&1
pwd
kustomize build . 2>&1 | head -100
cd ../../.. 2>&1
echo ""
# Alternative: Look at what the validation script does exactly
echo "=== Check if validation script successfully builds components ==="
bash ./hack/validate-components.sh 2>&1 | tail -50Repository: trustyai-explainability/trustyai-service-operator
Length of output: 480
roleRef.name hardcodes the prefixed name instead of using the short name — breaks kustomize's namePrefix transformation.
The ClusterRole in manager-rbac.yaml is named gorch-manager-role (short name), but this binding's roleRef.name is trustyai-service-operator-gorch-manager-role (hardcoded prefix). When overlays apply namePrefix: trustyai-service-operator-, kustomize transforms the ClusterRole name to trustyai-service-operator-gorch-manager-role. However, because roleRef.name already contains the full prefixed name instead of the short name, kustomize's name reference transformer cannot correlate the reference with the original resource name, leading to mismatches in standalone component builds and fragile deployments.
The roleRef should use the short name to align with how kustomize applies namePrefix:
Proposed fix
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
- name: trustyai-service-operator-gorch-manager-role
+ name: gorch-manager-roleThis pattern affects all component rolebindings (evalhub, job-mgr, lmes, nemo-guardrails, tas).
📝 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.
Suggested change
| roleRef: | |
| apiGroup: rbac.authorization.k8s.io | |
| kind: ClusterRole | |
| name: trustyai-service-operator-gorch-manager-role | |
| roleRef: | |
| apiGroup: rbac.authorization.k8s.io | |
| kind: ClusterRole | |
| name: gorch-manager-role |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/components/gorch/rbac/manager-rolebinding.yaml` around lines 12 - 15,
Update the roleRef.name in the manager RoleBinding so it references the short
ClusterRole name instead of the already-prefixed name: change roleRef.name in
manager-rolebinding.yaml from "trustyai-service-operator-gorch-manager-role" to
"gorch-manager-role" (keep apiGroup and kind unchanged) so kustomize namePrefix
can correctly transform and match the ClusterRole; apply the same fix pattern to
other component rolebindings (evalhub, job-mgr, lmes, nemo-guardrails, tas)
where roleRef.name is hardcoded with the prefix.
Contributor
Author
There was a problem hiding this comment.
When I used the shortname in the roleRef's the cluster role bindings broke, because it was trying to correlate the short name against the full qualified that the role receives when it's created on the cluster. This approach was the only one that worked.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
kustomize namePrefix roleRef transformation ClusterRoleBinding
💡 Result:
namePrefix renames metadata.name. To keep ClusterRoleBinding.roleRef.name in sync, Kustomize relies on its NameReference transformer (it rewrites “name fields that refer to other objects”). The transformer is configured via TransformerConfig.nameReference. [1]
If your ClusterRoleBinding.roleRef.name is not being updated, add an explicit NameReference config (and include it under configurations:):
# kustomizeconfig.yaml
nameReference:
- kind: ClusterRole
fieldSpecs:
- kind: ClusterRoleBinding
path: roleRef/name
- kind: RoleBinding
path: roleRef/name# kustomization.yaml
namePrefix: myprefix-
resources:
- clusterrole.yaml
- clusterrolebinding.yaml
configurations:
- kustomizeconfig.yamlAlso note: in Kubernetes, roleRef is immutable on (Cluster)RoleBindings—so if you already applied the binding once, and then Kustomize later changes roleRef.name (e.g., due to a new prefix), kubectl apply will fail; you must delete/recreate the binding. [2]
Sources: [1] [2]
@RobGeada, thanks for clarifying! Your experience makes perfect sense.
Kustomize's namePrefix automatically transforms metadata.name fields, but roleRef.name requires explicit NameReference transformer configuration to be updated. Without that configuration, roleRef.name stays as-written in the YAML, causing exactly the mismatch you encountered.
Your hardcoded approach works because it bypasses the need for transformation—you're directly specifying the final prefixed name that will exist on the cluster.
If you wanted to use short names in the future (keeping the manifests cleaner), you'd need to add a NameReference configuration:
# config/kustomizeconfig.yaml
nameReference:
- kind: ClusterRole
fieldSpecs:
- kind: ClusterRoleBinding
path: roleRef/nameThen reference it in your kustomization:
configurations:
- kustomizeconfig.yamlHowever, since your current approach is working and the repo doesn't have this configuration in place, there's no need to change it. The hardcoded names are explicit and avoid the transformation complexity.
🧠 Learnings used
Learnt from: scheruku-rh
Repo: trustyai-explainability/trustyai-service-operator PR: 652
File: config/rbac/evalhub/evalhub_secrets_reader_role.yaml:1-13
Timestamp: 2026-02-25T12:57:09.782Z
Learning: In the trustyai-service-operator repository, RBAC resources in config/rbac/ are deployed through config/base/kustomization.yaml which applies namePrefix: trustyai-service-operator-. ClusterRole names in YAML manifests should use short names (e.g., evalhub-secrets-reader) as kustomize automatically prefixes them at deployment time to match Go constants (e.g., trustyai-service-operator-evalhub-secrets-reader).
Comment on lines
+61
to
+70
| mkdir -p "${component_dir}/rbac" | ||
|
|
||
| # 1. Move CRD file (skip for job-mgr as it shares CRD with lmes) | ||
| # Auto-generated CRDs are moved from config/crd/bases/ to component directories | ||
| if [[ ! -z ${crd_pattern} ]]; then | ||
| echo " - Moving CRD (pattern: ${crd_pattern})" | ||
| crd_file=$(find "${CRD_BASES_DIR}" -name "*${crd_pattern}.yaml" 2>/dev/null | head -1) | ||
| if [[ -n "${crd_file}" && -f "${crd_file}" ]]; then | ||
| mv -f "${crd_file}" "${component_dir}/crd/$(basename "${crd_file}")" | ||
| echo " ✓ Moved: $(basename "${crd_file}")" |
Contributor
There was a problem hiding this comment.
Create the crd directory before moving CRD files.
mv to ${component_dir}/crd/... can fail when that directory is absent.
🛠️ Proposed fix
- mkdir -p "${component_dir}/rbac"
+ mkdir -p "${component_dir}/rbac" "${component_dir}/crd"📝 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.
Suggested change
| mkdir -p "${component_dir}/rbac" | |
| # 1. Move CRD file (skip for job-mgr as it shares CRD with lmes) | |
| # Auto-generated CRDs are moved from config/crd/bases/ to component directories | |
| if [[ ! -z ${crd_pattern} ]]; then | |
| echo " - Moving CRD (pattern: ${crd_pattern})" | |
| crd_file=$(find "${CRD_BASES_DIR}" -name "*${crd_pattern}.yaml" 2>/dev/null | head -1) | |
| if [[ -n "${crd_file}" && -f "${crd_file}" ]]; then | |
| mv -f "${crd_file}" "${component_dir}/crd/$(basename "${crd_file}")" | |
| echo " ✓ Moved: $(basename "${crd_file}")" | |
| mkdir -p "${component_dir}/rbac" "${component_dir}/crd" | |
| # 1. Move CRD file (skip for job-mgr as it shares CRD with lmes) | |
| # Auto-generated CRDs are moved from config/crd/bases/ to component directories | |
| if [[ ! -z ${crd_pattern} ]]; then | |
| echo " - Moving CRD (pattern: ${crd_pattern})" | |
| crd_file=$(find "${CRD_BASES_DIR}" -name "*${crd_pattern}.yaml" 2>/dev/null | head -1) | |
| if [[ -n "${crd_file}" && -f "${crd_file}" ]]; then | |
| mv -f "${crd_file}" "${component_dir}/crd/$(basename "${crd_file}")" | |
| echo " ✓ Moved: $(basename "${crd_file}")" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/generate-components.sh` around lines 61 - 70, The script moves CRD files
into "${component_dir}/crd" but never creates that directory, causing mv to
fail; before the mv call in the block handling crd_pattern (the section using
crd_file, CRD_BASES_DIR, and the mv -f "${crd_file}"
"${component_dir}/crd/$(basename "${crd_file}")"), ensure the target directory
exists by creating "${component_dir}/crd" (e.g., mkdir -p
"${component_dir}/crd") right after creating "${component_dir}/rbac" and before
the find/mv logic so the mv cannot fail due to a missing directory.
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: trustyai-service-operator-${component}-manager-role |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the size and content of the script around line 101
wc -l hack/generate-components.shRepository: trustyai-explainability/trustyai-service-operator
Length of output: 124
🏁 Script executed:
# Check what hack/generate-components.sh generates
sed -n '90,110p' hack/generate-components.shRepository: trustyai-explainability/trustyai-service-operator
Length of output: 774
🏁 Script executed:
# Look at the broader context of the script
head -50 hack/generate-components.shRepository: trustyai-explainability/trustyai-service-operator
Length of output: 1612
🏁 Script executed:
# Check the kustomization configuration
cat config/base/kustomization.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 1082
🏁 Script executed:
# Compare with actual RBAC files in config/rbac/
find config/rbac -name "*.yaml" -type f | head -20Repository: trustyai-explainability/trustyai-service-operator
Length of output: 139
🏁 Script executed:
# Check one of the actual RBAC files to see how names are structured
find config/rbac -name "*role*.yaml" -type f | xargs head -20Repository: trustyai-explainability/trustyai-service-operator
Length of output: 139
🏁 Script executed:
# Find all kustomization.yaml files
find config -name "kustomization.yaml" -type fRepository: trustyai-explainability/trustyai-service-operator
Length of output: 930
🏁 Script executed:
# Check the complete config/base/kustomization.yaml file
cat config/base/kustomization.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 1082
🏁 Script executed:
# Look at the overall config directory structure
tree -L 3 config/ | head -50Repository: trustyai-explainability/trustyai-service-operator
Length of output: 1254
🏁 Script executed:
# Check if namePrefix is in overlays
find config -name "kustomization.yaml" -type f -exec grep -l "namePrefix" {} \;Repository: trustyai-explainability/trustyai-service-operator
Length of output: 350
🏁 Script executed:
# Get the complete context of hack/generate-components.sh
cat hack/generate-components.shRepository: trustyai-explainability/trustyai-service-operator
Length of output: 4129
🏁 Script executed:
# Check base-rbac kustomization for namePrefix
cat config/base-rbac/kustomization.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 344
🏁 Script executed:
# Check a component kustomization file
cat config/components/evalhub/kustomization.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 643
🏁 Script executed:
# Check if overlays apply namePrefix
grep -r "namePrefix" config/overlays/Repository: trustyai-explainability/trustyai-service-operator
Length of output: 584
🏁 Script executed:
# Check the generated manager-rbac.yaml to understand what's created
cat config/components/evalhub/rbac/manager-rbac.yaml | head -30Repository: trustyai-explainability/trustyai-service-operator
Length of output: 531
🏁 Script executed:
# Check the manager-rolebinding.yaml in a component to see what's actually generated
cat config/components/evalhub/rbac/manager-rolebinding.yamlRepository: trustyai-explainability/trustyai-service-operator
Length of output: 675
Use the short ClusterRole name in roleRef.
The generated ClusterRole uses the short name pattern (evalhub-manager-role), but the ClusterRoleBinding's roleRef.name is hardcoded with the prefix (trustyai-service-operator-${component}-manager-role). This creates an inconsistency in the source manifests.
For Kustomize best practices, both should use short names in the source manifests and let namePrefix (applied at overlay level) handle the full naming consistently:
🛠️ Proposed fix
- name: trustyai-service-operator-${component}-manager-role
+ name: ${component}-manager-role📝 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.
Suggested change
| name: trustyai-service-operator-${component}-manager-role | |
| name: ${component}-manager-role |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/generate-components.sh` at line 101, The ClusterRoleBinding's
roleRef.name is using the full prefixed string
("trustyai-service-operator-${component}-manager-role") while the generated
ClusterRole uses the short name pattern ("${component}-manager-role"); update
the roleRef.name to use the same short name pattern as the ClusterRole (i.e.,
"${component}-manager-role" or the existing short-name variable) so kustomize
namePrefix can be applied consistently; modify the roleRef.name value in the
manifest generation logic that emits roleRef to match the ClusterRole name
generation (referencing the ClusterRole name generation symbol/pattern and
roleRef.name).
RobGeada
force-pushed
the
ControllerSeparation
branch
from
53763d5 to
0632108
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
hack/kubebuilder-rbac-to-yaml.sh (2)
13-15:⚠️ Potential issue | 🟠 MajorFix temp-file portability and trap quoting.
Line 13 and Line 14 use non-portable
mktempcalls, and Line 15 uses a double-quoted trap command that expands too early.Suggested fix
-TEMP_FILE=$(mktemp) -TEMP_SEEN=$(mktemp) -trap "rm -f ${TEMP_FILE} ${TEMP_SEEN}" EXIT +TEMP_FILE="$(mktemp "${TMPDIR:-/tmp}/trustyai-rbac.XXXXXX")" +TEMP_SEEN="$(mktemp "${TMPDIR:-/tmp}/trustyai-rbac-seen.XXXXXX")" +trap 'rm -f "$TEMP_FILE" "$TEMP_SEEN"' EXIT#!/usr/bin/env bash set -euo pipefail cat -n hack/kubebuilder-rbac-to-yaml.sh | sed -n '10,18p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/kubebuilder-rbac-to-yaml.sh` around lines 13 - 15, TEMP_FILE and TEMP_SEEN are created with non-portable mktemp usage and the trap uses double quotes causing early expansion; replace the mktemp calls in the script (symbols: TEMP_FILE and TEMP_SEEN) with a portable pattern that falls back for macOS (e.g. try mktemp normally and if it fails use mktemp -t with a prefix) and update the trap invocation (symbol: trap) to use single quotes and quoted variables (e.g. trap 'rm -f "$TEMP_FILE" "$TEMP_SEEN"' EXIT) so the temp file names are expanded at trap execution time, not at trap definition.
66-104:⚠️ Potential issue | 🟠 MajorGuard non-resource URL rules from emitting
apiGroups/resources.Line 67 currently forces
apiGroupseven forurls=-only markers, which can produce invalid RBAC rules.Suggested fix
- # Output YAML rule - echo "- apiGroups:" - if [[ -n "${groups}" ]]; then - IFS=';' read -ra GROUP_ARRAY <<< "${groups}" - for group in "${GROUP_ARRAY[@]}"; do - # Handle empty group (core API) - if [[ "${group}" == "core" || "${group}" == '""' || -z "${group}" ]]; then - echo ' - ""' - else - echo " - ${group}" - fi - done - else - echo ' - ""' - fi - - if [[ -n "${resources}" ]]; then + # Output YAML rule + if [[ -n "${urls}" && -n "${resources}" ]]; then + echo "Error: invalid marker uses both resources and urls: ${marker}" >&2 + exit 1 + fi + + if [[ -n "${urls}" ]]; then + echo "- nonResourceURLs:" + IFS=';' read -ra URL_ARRAY <<< "${urls}" + for url in "${URL_ARRAY[@]}"; do + echo " - ${url}" + done + else + echo "- apiGroups:" + if [[ -n "${groups}" ]]; then + IFS=';' read -ra GROUP_ARRAY <<< "${groups}" + for group in "${GROUP_ARRAY[@]}"; do + if [[ "${group}" == "core" || "${group}" == '""' || -z "${group}" ]]; then + echo ' - ""' + else + echo " - ${group}" + fi + done + else + echo ' - ""' + fi + + if [[ -n "${resources}" ]]; then + echo " resources:" + IFS=';' read -ra RESOURCE_ARRAY <<< "${resources}" + for resource in "${RESOURCE_ARRAY[@]}"; do + echo " - ${resource}" + done + fi + fi + + if [[ -n "${verbs}" ]]; then + echo " verbs:" + IFS=';' read -ra VERB_ARRAY <<< "${verbs}" + for verb in "${VERB_ARRAY[@]}"; do + echo " - ${verb}" + done + fi - echo " resources:" - IFS=';' read -ra RESOURCE_ARRAY <<< "${resources}" - for resource in "${RESOURCE_ARRAY[@]}"; do - echo " - ${resource}" - done - fi - - if [[ -n "${verbs}" ]]; then - echo " verbs:" - IFS=';' read -ra VERB_ARRAY <<< "${verbs}" - for verb in "${VERB_ARRAY[@]}"; do - echo " - ${verb}" - done - fi - - if [[ -n "${urls}" ]]; then - echo " nonResourceURLs:" - IFS=';' read -ra URL_ARRAY <<< "${urls}" - for url in "${URL_ARRAY[@]}"; do - echo " - ${url}" - done - fi#!/usr/bin/env bash set -euo pipefail python - <<'PY' from pathlib import Path p = Path("hack/kubebuilder-rbac-to-yaml.sh").read_text().splitlines() for i,l in enumerate(p,1): if 'echo "- apiGroups:"' in l or 'if [[ -n "${urls}"' in l: print(f"{i}: {l}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/kubebuilder-rbac-to-yaml.sh` around lines 66 - 104, The script always emits the leading "echo \"- apiGroups:\"" even when only non-resource URL rules (urls) are present, producing invalid RBAC entries; change the logic around the top-level emission so the "- apiGroups:" line and subsequent apiGroups/resources/verbs blocks are only printed when at least one of groups, resources, or verbs is non-empty (i.e., not for urls-only markers). Specifically, update the conditional that currently prints echo "- apiGroups:" and the handling of ${groups}, ${resources}, and ${verbs} so that if only ${urls} is set the script skips printing apiGroups/resources/verbs and only emits the nonResourceURLs block; refer to the variables/groups of GROUP_ARRAY, RESOURCE_ARRAY, VERB_ARRAY and the urls handling (URL_ARRAY) in the shown diff to locate the code to change.hack/extract-component-rbac.sh (1)
26-27:⚠️ Potential issue | 🟡 MinorUse portable temp-file creation and safe trap quoting.
Line 26/27 has the same portability and trap-expansion issue; this can break on BSD/macOS and weakens cleanup reliability.
Suggested fix
-TEMP_RULES=$(mktemp) -trap "rm -f ${TEMP_RULES}" EXIT +TEMP_RULES="$(mktemp "${TMPDIR:-/tmp}/trustyai-component-rbac.XXXXXX")" +trap 'rm -f "$TEMP_RULES"' EXIT#!/usr/bin/env bash set -euo pipefail cat -n hack/extract-component-rbac.sh | sed -n '24,30p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/extract-component-rbac.sh` around lines 26 - 27, TEMP_RULES is created with mktemp in a non-portable way and the trap is double-quoted so it can expand early; update the TEMP_RULES creation to a portable fallback (e.g. try mktemp and fall back to mktemp -t or a template) so it works on BSD/macOS, ensure usages quote the variable (use "$TEMP_RULES"), and change the trap to a single-quoted form that references the variable safely (e.g. trap 'rm -f "$TEMP_RULES"' EXIT) so cleanup runs reliably; locate the TEMP_RULES assignment and the trap in hack/extract-component-rbac.sh to apply these changes.config/components/evalhub/rbac/manager-rbac.yaml (1)
96-149:⚠️ Potential issue | 🟠 MajorReduce RBAC write privileges for RBAC resources.
Lines 96-149 grant high-impact write access (
clusterrolebindings,roles,rolebindings). This is broader than typical controller needs and increases privilege-escalation risk.Hardening baseline
- - apiGroups: - - rbac.authorization.k8s.io - resources: - - clusterrolebindings - verbs: - - get - - list - - watch - - create - - update - - delete#!/usr/bin/env bash set -euo pipefail # Inspect current RBAC write grants in this manifest cat -n config/components/evalhub/rbac/manager-rbac.yaml | sed -n '90,155p' # Check if RBAC object manipulation appears in Go codepaths rg -n --type=go -C2 'rbacv1\.(ClusterRoleBinding|RoleBinding|Role)'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/components/evalhub/rbac/manager-rbac.yaml` around lines 96 - 149, The manifest grants broad write rights to RBAC objects (clusterrolebindings, roles, rolebindings); change the verbs for these resources in manager-rbac.yaml to least-privilege (remove create/update/patch/delete and restrict to get/list/watch) unless your controller explicitly needs to mutate them; if mutation is required, narrow scope (e.g., namespace-scoped Role/RoleBinding instead of ClusterRoleBinding) or create a separate narrowly-scoped ClusterRole with only the exact verbs and resources needed and add a brief code comment justifying why the write verbs remain; also run the suggested grep (rbacv1.(ClusterRoleBinding|RoleBinding|Role)) to verify which code paths actually require mutation before leaving write permissions.hack/generate-components.sh (3)
98-102:⚠️ Potential issue | 🟠 MajorUse the short ClusterRole name in
roleRef.name.Line 101 hardcodes the prefixed name. Source manifests should keep the short name so kustomize can apply
namePrefixconsistently.🛠️ Proposed fix
roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: trustyai-service-operator-${component}-manager-role + name: ${component}-manager-role#!/bin/bash # Verify manager-rbac ClusterRole name matches manager-rolebinding roleRef.name in source manifests. for comp in config/components/*; do [ -d "${comp}/rbac" ] || continue role_file="${comp}/rbac/manager-rbac.yaml" binding_file="${comp}/rbac/manager-rolebinding.yaml" [ -f "$role_file" ] && [ -f "$binding_file" ] || continue role_name=$(awk ' $1=="kind:" && $2=="ClusterRole"{in_role=1} in_role && $1=="name:"{print $2; exit} ' "$role_file") ref_name=$(awk ' $1=="roleRef:"{in_ref=1} in_ref && $1=="name:"{print $2; exit} ' "$binding_file") if [ -n "$role_name" ] && [ -n "$ref_name" ] && [ "$role_name" != "$ref_name" ]; then echo "Mismatch in $(basename "$comp"): ClusterRole=$role_name, roleRef.name=$ref_name" fi doneBased on learnings: In this repository, ClusterRole names in source YAML should be short names because kustomize
namePrefixaddstrustyai-service-operator-at deployment time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 98 - 102, The roleRef.name in the manifest is hardcoded with the prefixed ClusterRole name; update the source manifests to use the short ClusterRole name so kustomize namePrefix can add trustyai-service-operator- at deploy time—specifically ensure the ClusterRole resource (e.g., in manager-rbac.yaml) and the RoleBinding/ClusterRoleBinding roleRef.name (e.g., in manager-rolebinding.yaml) match on the short name (no trustyai-service-operator- prefix) and that roleRef: name uses that short identifier consistently.
61-70:⚠️ Potential issue | 🟠 MajorCreate the
crddirectory before moving CRDs.Line 61 creates only
rbac, but Line 69 moves into${component_dir}/crd/...; this can fail on clean runs underset -e.🛠️ Proposed fix
- mkdir -p "${component_dir}/rbac" + mkdir -p "${component_dir}/rbac" "${component_dir}/crd"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 61 - 70, The script creates only the rbac directory but later attempts to move CRD files into "${component_dir}/crd", which can fail; update the logic in generate-components.sh so before moving files (the block using crd_pattern and the mv -f into "${component_dir}/crd/$(basename "${crd_file}")") you create the target directory (e.g., run mkdir -p "${component_dir}/crd") when crd_pattern is non-empty so the mv always has an existing destination.
71-73:⚠️ Potential issue | 🟠 MajorFail fast when a required CRD is missing.
Line 72 only logs and continues, which can produce incomplete components and defer failure to apply/build time. Keep the existing skip behavior for
job-mgrvia emptycrd_pattern, but error for missing required CRDs.🛠️ Proposed fix
- else - echo " ✗ No CRDs for pattern '${crd_pattern}'" - fi + else + echo " ✗ Error: No CRDs for pattern '${crd_pattern}' for component '${component}'" >&2 + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 71 - 73, The script currently only logs "No CRDs for pattern '${crd_pattern}'" and continues, which can lead to incomplete outputs; modify the else branch handling that message so it fails fast for required CRDs: if crd_pattern is empty (the existing skip behavior used for job-mgr) keep the current echo and continue, but if crd_pattern is non-empty then log an error and exit with a non-zero status (e.g., exit 1). Update the block that prints "✗ No CRDs for pattern '${crd_pattern}'" to check the crd_pattern variable and call exit 1 when missing required CRDs.Makefile (1)
303-329:⚠️ Potential issue | 🟠 MajorUnify overlay default behavior and
basehandling inmanifest-gen.Line 310 advertises
base (default), Line 323 actually defaults toconfig/overlays/testing, and Lines 314-328 allowOVERLAY=basebut still build fromconfig/overlays/base. These paths/messages are inconsistent and can produce unexpected failures.✏️ Suggested consolidation
+DEFAULT_OVERLAY ?= testing + manifest-gen: kustomize ## Generate deployment manifests. Usage: make manifest-gen NAMESPACE=<namespace> [OVERLAY=<overlay>] [OPERATOR_IMAGE=<image>] `@if` [ -z "$(NAMESPACE)" ]; then \ echo "Error: NAMESPACE argument is required"; \ @@ - echo " base (default)"; \ + echo " $(DEFAULT_OVERLAY) (default)"; \ ls -1 config/overlays/ 2>/dev/null | sed 's/^/ /'; \ exit 1; \ fi - `@if` [ ! -z "$(OVERLAY)" ] && [ "$(OVERLAY)" != "base" ] && [ ! -d "config/overlays/$(OVERLAY)" ]; then \ + `@SELECTED_OVERLAY`="$${OVERLAY:-$(DEFAULT_OVERLAY)}"; \ + if [ "$$SELECTED_OVERLAY" != "base" ] && [ ! -d "config/overlays/$$SELECTED_OVERLAY" ]; then \ echo "Error: Overlay '$(OVERLAY)' not found"; \ echo "Available overlays:"; \ echo " base"; \ ls -1 config/overlays/ 2>/dev/null | sed 's/^/ /' || echo " (no overlays found)"; \ exit 1; \ fi `@mkdir` -p release - `@if` [ -z "$(OVERLAY)" ]; then \ - echo "Building from config/overlays/testing..."; \ - $(KUSTOMIZE) build config/overlays/testing | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_bundle.yaml; \ + `@SELECTED_OVERLAY`="$${OVERLAY:-$(DEFAULT_OVERLAY)}"; \ + if [ "$$SELECTED_OVERLAY" = "base" ]; then OVERLAY_PATH="config/base"; else OVERLAY_PATH="config/overlays/$$SELECTED_OVERLAY"; fi; \ + if [ -z "$(OVERLAY)" ]; then \ + echo "Building from $$OVERLAY_PATH..."; \ + $(KUSTOMIZE) build $$OVERLAY_PATH | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_bundle.yaml; \ echo "✓ Release manifest generated at release/trustyai_bundle.yaml"; \ else \ - echo "Building from config/overlays/$(OVERLAY)..."; \ - $(KUSTOMIZE) build config/overlays/$(OVERLAY) | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_$(OVERLAY)_bundle.yaml; \ - echo "✓ Release manifest generated at release/trustyai_$(OVERLAY)_bundle.yaml"; \ + echo "Building from $$OVERLAY_PATH..."; \ + $(KUSTOMIZE) build $$OVERLAY_PATH | sed "s|namespace: system|namespace: $(NAMESPACE)|g" | sed "s|quay.io/trustyai/trustyai-service-operator:latest|$(OPERATOR_IMAGE)|g" > release/trustyai_$$SELECTED_OVERLAY_bundle.yaml; \ + echo "✓ Release manifest generated at release/trustyai_$$SELECTED_OVERLAY_bundle.yaml"; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 303 - 329, The manifest-gen Makefile logic is inconsistent: it claims "base (default)" but actually builds from config/overlays/testing when OVERLAY is empty and treats OVERLAY=base specially; update the conditional that checks if OVERLAY is empty (and the echo messages) so the true default is "base" (i.e., build from config/overlays/base when OVERLAY is unset), ensure the help text and "Available overlays" output still list base as default, and keep the existing branch that builds config/overlays/$(OVERLAY) when OVERLAY is provided; touch the KUSTOMIZE build lines and the release output filenames (release/trustyai_bundle.yaml vs release/trustyai_$(OVERLAY)_bundle.yaml) to reflect this unified default behavior.
🧹 Nitpick comments (2)
hack/generate-components.sh (2)
16-16: Remove unusedRBAC_DIRdeclaration.
RBAC_DIRis currently unused (SC2034). Dropping it reduces script noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` at line 16, Remove the unused variable declaration RBAC_DIR="${CONFIG_DIR}/rbac" from the script; locate the line that defines RBAC_DIR in hack/generate-components.sh and delete it (ensure no other references to RBAC_DIR exist), leaving CONFIG_DIR intact and the script otherwise unchanged.
116-118: Make cleanup idempotent withrm -f.Lines 117-118 will fail on reruns if files are already absent due to
set -e.♻️ Proposed fix
-rm ${CONFIG_DIR}/rbac/role.yaml -rm ${CONFIG_DIR}/rbac/kustomization.yaml +rm -f "${CONFIG_DIR}/rbac/role.yaml" +rm -f "${CONFIG_DIR}/rbac/kustomization.yaml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 116 - 118, Replace the non-idempotent removals so the cleanup won’t fail on reruns: update the two rm invocations that reference ${CONFIG_DIR}/rbac/role.yaml and ${CONFIG_DIR}/rbac/kustomization.yaml to use the forced/ignore-missing form (rm -f) and quote the variable expansion (e.g., "${CONFIG_DIR}/rbac/role.yaml") to handle paths with spaces and avoid set -e terminating the script when files are already absent.
🤖 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/components/job-mgr/rbac/manager-rolebinding.yaml`:
- Around line 12-15: Update the roleRef.name value so it exactly matches the
existing ClusterRole short name: change the current roleRef.name value from
"trustyai-service-operator-job-mgr-manager-role" to "job-mgr-manager-role"
(ensure the roleRef block that contains apiGroup: rbac.authorization.k8s.io,
kind: ClusterRole and name: is updated accordingly).
In `@Makefile`:
- Line 99: The manifests pipeline is failing on reruns because
hack/generate-components.sh deletes config/rbac/role.yaml and
config/rbac/kustomization.yaml with plain rm (lines 117–118), which errors if
files are missing; update those two rm invocations to use rm -f so the deletion
is idempotent and manifests target (dependent via Makefile ->
components-generate) can run repeatedly without error.
---
Duplicate comments:
In `@config/components/evalhub/rbac/manager-rbac.yaml`:
- Around line 96-149: The manifest grants broad write rights to RBAC objects
(clusterrolebindings, roles, rolebindings); change the verbs for these resources
in manager-rbac.yaml to least-privilege (remove create/update/patch/delete and
restrict to get/list/watch) unless your controller explicitly needs to mutate
them; if mutation is required, narrow scope (e.g., namespace-scoped
Role/RoleBinding instead of ClusterRoleBinding) or create a separate
narrowly-scoped ClusterRole with only the exact verbs and resources needed and
add a brief code comment justifying why the write verbs remain; also run the
suggested grep (rbacv1.(ClusterRoleBinding|RoleBinding|Role)) to verify which
code paths actually require mutation before leaving write permissions.
In `@hack/extract-component-rbac.sh`:
- Around line 26-27: TEMP_RULES is created with mktemp in a non-portable way and
the trap is double-quoted so it can expand early; update the TEMP_RULES creation
to a portable fallback (e.g. try mktemp and fall back to mktemp -t or a
template) so it works on BSD/macOS, ensure usages quote the variable (use
"$TEMP_RULES"), and change the trap to a single-quoted form that references the
variable safely (e.g. trap 'rm -f "$TEMP_RULES"' EXIT) so cleanup runs reliably;
locate the TEMP_RULES assignment and the trap in hack/extract-component-rbac.sh
to apply these changes.
In `@hack/generate-components.sh`:
- Around line 98-102: The roleRef.name in the manifest is hardcoded with the
prefixed ClusterRole name; update the source manifests to use the short
ClusterRole name so kustomize namePrefix can add trustyai-service-operator- at
deploy time—specifically ensure the ClusterRole resource (e.g., in
manager-rbac.yaml) and the RoleBinding/ClusterRoleBinding roleRef.name (e.g., in
manager-rolebinding.yaml) match on the short name (no trustyai-service-operator-
prefix) and that roleRef: name uses that short identifier consistently.
- Around line 61-70: The script creates only the rbac directory but later
attempts to move CRD files into "${component_dir}/crd", which can fail; update
the logic in generate-components.sh so before moving files (the block using
crd_pattern and the mv -f into "${component_dir}/crd/$(basename "${crd_file}")")
you create the target directory (e.g., run mkdir -p "${component_dir}/crd") when
crd_pattern is non-empty so the mv always has an existing destination.
- Around line 71-73: The script currently only logs "No CRDs for pattern
'${crd_pattern}'" and continues, which can lead to incomplete outputs; modify
the else branch handling that message so it fails fast for required CRDs: if
crd_pattern is empty (the existing skip behavior used for job-mgr) keep the
current echo and continue, but if crd_pattern is non-empty then log an error and
exit with a non-zero status (e.g., exit 1). Update the block that prints "✗ No
CRDs for pattern '${crd_pattern}'" to check the crd_pattern variable and call
exit 1 when missing required CRDs.
In `@hack/kubebuilder-rbac-to-yaml.sh`:
- Around line 13-15: TEMP_FILE and TEMP_SEEN are created with non-portable
mktemp usage and the trap uses double quotes causing early expansion; replace
the mktemp calls in the script (symbols: TEMP_FILE and TEMP_SEEN) with a
portable pattern that falls back for macOS (e.g. try mktemp normally and if it
fails use mktemp -t with a prefix) and update the trap invocation (symbol: trap)
to use single quotes and quoted variables (e.g. trap 'rm -f "$TEMP_FILE"
"$TEMP_SEEN"' EXIT) so the temp file names are expanded at trap execution time,
not at trap definition.
- Around line 66-104: The script always emits the leading "echo \"-
apiGroups:\"" even when only non-resource URL rules (urls) are present,
producing invalid RBAC entries; change the logic around the top-level emission
so the "- apiGroups:" line and subsequent apiGroups/resources/verbs blocks are
only printed when at least one of groups, resources, or verbs is non-empty
(i.e., not for urls-only markers). Specifically, update the conditional that
currently prints echo "- apiGroups:" and the handling of ${groups},
${resources}, and ${verbs} so that if only ${urls} is set the script skips
printing apiGroups/resources/verbs and only emits the nonResourceURLs block;
refer to the variables/groups of GROUP_ARRAY, RESOURCE_ARRAY, VERB_ARRAY and the
urls handling (URL_ARRAY) in the shown diff to locate the code to change.
In `@Makefile`:
- Around line 303-329: The manifest-gen Makefile logic is inconsistent: it
claims "base (default)" but actually builds from config/overlays/testing when
OVERLAY is empty and treats OVERLAY=base specially; update the conditional that
checks if OVERLAY is empty (and the echo messages) so the true default is "base"
(i.e., build from config/overlays/base when OVERLAY is unset), ensure the help
text and "Available overlays" output still list base as default, and keep the
existing branch that builds config/overlays/$(OVERLAY) when OVERLAY is provided;
touch the KUSTOMIZE build lines and the release output filenames
(release/trustyai_bundle.yaml vs release/trustyai_$(OVERLAY)_bundle.yaml) to
reflect this unified default behavior.
---
Nitpick comments:
In `@hack/generate-components.sh`:
- Line 16: Remove the unused variable declaration RBAC_DIR="${CONFIG_DIR}/rbac"
from the script; locate the line that defines RBAC_DIR in
hack/generate-components.sh and delete it (ensure no other references to
RBAC_DIR exist), leaving CONFIG_DIR intact and the script otherwise unchanged.
- Around line 116-118: Replace the non-idempotent removals so the cleanup won’t
fail on reruns: update the two rm invocations that reference
${CONFIG_DIR}/rbac/role.yaml and ${CONFIG_DIR}/rbac/kustomization.yaml to use
the forced/ignore-missing form (rm -f) and quote the variable expansion (e.g.,
"${CONFIG_DIR}/rbac/role.yaml") to handle paths with spaces and avoid set -e
terminating the script when files are already absent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (73)
Makefileapi/evalhub/v1alpha1/zz_generated.deepcopy.goapi/gorch/v1alpha1/zz_generated.deepcopy.goapi/lmes/v1alpha1/zz_generated.deepcopy.goapi/nemo_guardrails/v1alpha1/zz_generated.deepcopy.goapi/tas/v1/zz_generated.deepcopy.goapi/tas/v1alpha1/zz_generated.deepcopy.goconfig/base-rbac/auth-delegator.ymlconfig/base-rbac/auth_proxy_client_clusterrole.yamlconfig/base-rbac/auth_proxy_role.yamlconfig/base-rbac/auth_proxy_role_binding.yamlconfig/base-rbac/auth_proxy_service.yamlconfig/base-rbac/kustomization.yamlconfig/base-rbac/leader_election_role.yamlconfig/base-rbac/leader_election_role_binding.yamlconfig/base-rbac/service_account.yamlconfig/base/kustomization.yamlconfig/components/evalhub/crd/trustyai.opendatahub.io_evalhubs.yamlconfig/components/evalhub/kustomization.yamlconfig/components/evalhub/rbac/evalhub_auth_reviewer_role.yamlconfig/components/evalhub/rbac/evalhub_job_config_binding.yamlconfig/components/evalhub/rbac/evalhub_job_config_role.yamlconfig/components/evalhub/rbac/evalhub_jobs_writer_binding.yamlconfig/components/evalhub/rbac/evalhub_jobs_writer_role.yamlconfig/components/evalhub/rbac/evalhub_mlflow_access_binding.yamlconfig/components/evalhub/rbac/evalhub_mlflow_access_role.yamlconfig/components/evalhub/rbac/evalhub_mlflow_jobs_binding.yamlconfig/components/evalhub/rbac/evalhub_mlflow_jobs_role.yamlconfig/components/evalhub/rbac/manager-rbac.yamlconfig/components/evalhub/rbac/manager-rolebinding.yamlconfig/components/gorch/crd/trustyai.opendatahub.io_guardrailsorchestrators.yamlconfig/components/gorch/kustomization.yamlconfig/components/gorch/rbac/manager-rbac.yamlconfig/components/gorch/rbac/manager-rolebinding.yamlconfig/components/job-mgr/kustomization.yamlconfig/components/job-mgr/rbac/manager-rbac.yamlconfig/components/job-mgr/rbac/manager-rolebinding.yamlconfig/components/lmes/configmaps/lmeval-tasks.yamlconfig/components/lmes/crd/trustyai.opendatahub.io_lmevaljobs.yamlconfig/components/lmes/kustomization.yamlconfig/components/lmes/rbac/default_lmeval_user_rolebinding.yamlconfig/components/lmes/rbac/manager-rbac.yamlconfig/components/lmes/rbac/manager-rolebinding.yamlconfig/components/lmes/rbac/non_admin_lmeval_role.yamlconfig/components/nemo-guardrails/crd/trustyai.opendatahub.io_nemoguardrails.yamlconfig/components/nemo-guardrails/kustomization.yamlconfig/components/nemo-guardrails/rbac/manager-rbac.yamlconfig/components/nemo-guardrails/rbac/manager-rolebinding.yamlconfig/components/nemo-guardrails/rbac/nemoguardrail_editor_role.yamlconfig/components/nemo-guardrails/rbac/nemoguardrail_viewer_role.yamlconfig/components/tas/crd/trustyai.opendatahub.io_trustyaiservices.yamlconfig/components/tas/kustomization.yamlconfig/components/tas/rbac/manager-rbac.yamlconfig/components/tas/rbac/manager-rolebinding.yamlconfig/components/tas/rbac/trustyaiservice_editor_role.yamlconfig/components/tas/rbac/trustyaiservice_viewer_role.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/overlays/lmes/kustomization.yamlconfig/overlays/mcp-guardrails/kustomization.yamlconfig/overlays/mcp-guardrails/params.envconfig/overlays/odh-kueue/kustomization.yamlconfig/overlays/odh/kustomization.yamlconfig/overlays/rhoai/kustomization.yamlconfig/overlays/testing/kustomization.yamlconfig/rbac/kustomization.yamlhack/boilerplate.go.txthack/boilerplate.yaml.txthack/extract-component-rbac.shhack/generate-component-kustomization.shhack/generate-components.shhack/kubebuilder-rbac-to-yaml.shhack/validate-components.sh
💤 Files with no reviewable changes (1)
- config/rbac/kustomization.yaml
✅ Files skipped from review due to trivial changes (6)
- hack/boilerplate.yaml.txt
- config/components/tas/crd/trustyai.opendatahub.io_trustyaiservices.yaml
- hack/boilerplate.go.txt
- config/crd/kustomization.yaml
- api/nemo_guardrails/v1alpha1/zz_generated.deepcopy.go
- config/components/gorch/crd/trustyai.opendatahub.io_guardrailsorchestrators.yaml
🚧 Files skipped from review as they are similar to previous changes (23)
- api/lmes/v1alpha1/zz_generated.deepcopy.go
- api/tas/v1/zz_generated.deepcopy.go
- hack/generate-component-kustomization.sh
- config/components/nemo-guardrails/crd/trustyai.opendatahub.io_nemoguardrails.yaml
- config/overlays/testing/kustomization.yaml
- config/components/nemo-guardrails/kustomization.yaml
- config/overlays/mcp-guardrails/params.env
- config/components/gorch/rbac/manager-rolebinding.yaml
- api/evalhub/v1alpha1/zz_generated.deepcopy.go
- config/overlays/lmes/kustomization.yaml
- config/components/nemo-guardrails/rbac/manager-rolebinding.yaml
- config/components/lmes/kustomization.yaml
- api/gorch/v1alpha1/zz_generated.deepcopy.go
- hack/validate-components.sh
- config/base-rbac/kustomization.yaml
- config/components/job-mgr/kustomization.yaml
- config/base/kustomization.yaml
- config/components/lmes/configmaps/lmeval-tasks.yaml
- config/manager/manager.yaml
- config/components/lmes/rbac/manager-rolebinding.yaml
- config/components/job-mgr/rbac/manager-rbac.yaml
- config/components/nemo-guardrails/rbac/manager-rbac.yaml
- config/components/gorch/rbac/manager-rbac.yaml
Comment on lines
+12
to
+15
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: trustyai-service-operator-job-mgr-manager-role |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the ClusterRole name and check if namePrefix is applied
echo "=== ClusterRole name in companion file ==="
cat config/components/job-mgr/rbac/manager-rbac.yaml 2>/dev/null | grep -A2 "^kind: ClusterRole" | grep "name:"
echo ""
echo "=== Check if component kustomization applies namePrefix ==="
cat config/components/job-mgr/kustomization.yaml 2>/dev/null | grep -A1 "namePrefix"
echo ""
echo "=== Check if base kustomization applies namePrefix ==="
cat config/base/kustomization.yaml 2>/dev/null | grep -A1 "namePrefix"
echo ""
echo "=== Full ClusterRole definition ==="
cat config/components/job-mgr/rbac/manager-rbac.yaml 2>/dev/null
echo ""
echo "=== Full ClusterRoleBinding definition ==="
cat config/components/job-mgr/rbac/manager-rolebinding.yaml 2>/dev/nullRepository: trustyai-explainability/trustyai-service-operator
Length of output: 1795
Fix roleRef.name to match the ClusterRole name: change from trustyai-service-operator-job-mgr-manager-role to job-mgr-manager-role.
The ClusterRole is defined with the short name job-mgr-manager-role, but the roleRef.name here references trustyai-service-operator-job-mgr-manager-role. This mismatch will cause the RoleBinding to fail at runtime because it will reference a non-existent ClusterRole. The roleRef.name must exactly match the actual ClusterRole name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/components/job-mgr/rbac/manager-rolebinding.yaml` around lines 12 -
15, Update the roleRef.name value so it exactly matches the existing ClusterRole
short name: change the current roleRef.name value from
"trustyai-service-operator-job-mgr-manager-role" to "job-mgr-manager-role"
(ensure the roleRef block that contains apiGroup: rbac.authorization.k8s.io,
kind: ClusterRole and name: is updated accordingly).
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
hack/generate-components.sh (3)
61-69:⚠️ Potential issue | 🔴 CriticalCreate the
crddirectory before moving CRD files.Line 61 only creates
rbac, but Line 69 moves into${component_dir}/crd/...; this can fail the script on the first CRD move.🛠️ Proposed fix
- mkdir -p "${component_dir}/rbac" + mkdir -p "${component_dir}/rbac" "${component_dir}/crd"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 61 - 69, The script creates the "${component_dir}/rbac" directory but never ensures the "${component_dir}/crd" directory exists before moving CRD files; update the setup so when crd_pattern is set you create the target CRD directory (e.g., run mkdir -p "${component_dir}/crd") before computing crd_file and calling mv -f; place this mkdir alongside the existing mkdir -p "${component_dir}/rbac" or immediately before the mv that uses crd_file so component_dir, crd_pattern, CRD_BASES_DIR, crd_file and the mv -f call succeed reliably.
71-73:⚠️ Potential issue | 🟠 MajorFail fast when a required CRD is missing.
At Line 72, warning-only behavior allows incomplete components to be generated and defers failures to later stages.
🛠️ Proposed fix
- else - echo " ✗ No CRDs for pattern '${crd_pattern}'" - fi + else + echo >&2 " ✗ Error: CRD not found for pattern '${crd_pattern}' (component: ${component})" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` around lines 71 - 73, The script currently only prints a warning when no CRDs match (echo "✗ No CRDs for pattern '${crd_pattern}'"); change this to fail fast by exiting with a non-zero status: after the echo (or instead of it) call exit 1 (or use a helper like die) so the generate-components.sh run stops immediately when crd_pattern yields no CRDs; reference the echo line and the crd_pattern variable to locate where to replace the warning-only behavior with an exit.
101-101:⚠️ Potential issue | 🟠 MajorUse the short role name in
roleRef.nameand let kustomize apply prefixes.Line 101 hardcodes a prefixed role name. Keep source manifests on short names so role and roleRef stay consistent before transforms.
Based on learnings: RBAC resources are prefixed by kustomize (namePrefix: trustyai-service-operator-), so source ClusterRole naming should stay short.🛠️ Proposed fix
- name: trustyai-service-operator-${component}-manager-role + name: ${component}-manager-roleRun this to verify each component’s
manager-rbac.yamlClusterRole name matchesmanager-rolebinding.yamlroleRef.namein source manifests (expected: no mismatches):#!/bin/bash set -euo pipefail find config/components -type f -path "*/rbac/manager-rolebinding.yaml" -print0 | \ while IFS= read -r -d '' binding; do comp_dir="$(dirname "$(dirname "$binding")")" role_file="${comp_dir}/rbac/manager-rbac.yaml" if [[ ! -f "${role_file}" ]]; then echo "Missing manager-rbac.yaml for ${comp_dir}" continue fi role_name="$(awk ' $1=="kind:" && $2=="ClusterRole" {in_cr=1} in_cr && $1=="name:" {print $2; exit} ' "${role_file}")" role_ref_name="$(awk ' $1=="roleRef:" {in_rr=1; next} in_rr && $1=="name:" {print $2; exit} ' "${binding}")" if [[ -n "${role_name}" && -n "${role_ref_name}" && "${role_name}" != "${role_ref_name}" ]]; then echo "Mismatch in ${comp_dir}: ClusterRole=${role_name}, roleRef=${role_ref_name}" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` at line 101, Summary: roleRef.name is hardcoded with the kustomize prefix; use the short role name so kustomize can apply namePrefix. Fix: in the source manifests (manager-rbac.yaml and manager-rolebinding.yaml) remove the hardcoded "trustyai-service-operator-" prefix from the ClusterRole name and from roleRef.name so both use the short role identifier (e.g., "manager-role" or the component-local short name) and rely on namePrefix: trustyai-service-operator- to add the prefix; ensure ClusterRole (kind: ClusterRole) name and roleRef.name match exactly after this change and re-run the provided verification script to confirm no mismatches.
🧹 Nitpick comments (1)
hack/generate-components.sh (1)
16-16: UseRBAC_DIRconsistently (or remove it).Line 16 defines
RBAC_DIR, but Line 117 bypasses it. Wiring it into the delete path resolves the unused-variable warning cleanly.♻️ Proposed fix
-rm -f ${CONFIG_DIR}/rbac/role.yaml +rm -f "${RBAC_DIR}/role.yaml"Also applies to: 117-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/generate-components.sh` at line 16, RBAC_DIR is defined but not used; update the delete invocation that currently bypasses RBAC_DIR (the delete path around the code referenced at line ~117) to use the RBAC_DIR variable instead of the hardcoded "${CONFIG_DIR}/rbac" (or any literal path), e.g. replace the hardcoded deletion target with "${RBAC_DIR}" so the script consistently uses the RBAC_DIR variable (ensure any surrounding quotes and variable expansions match existing usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hack/generate-components.sh`:
- Around line 61-69: The script creates the "${component_dir}/rbac" directory
but never ensures the "${component_dir}/crd" directory exists before moving CRD
files; update the setup so when crd_pattern is set you create the target CRD
directory (e.g., run mkdir -p "${component_dir}/crd") before computing crd_file
and calling mv -f; place this mkdir alongside the existing mkdir -p
"${component_dir}/rbac" or immediately before the mv that uses crd_file so
component_dir, crd_pattern, CRD_BASES_DIR, crd_file and the mv -f call succeed
reliably.
- Around line 71-73: The script currently only prints a warning when no CRDs
match (echo "✗ No CRDs for pattern '${crd_pattern}'"); change this to fail fast
by exiting with a non-zero status: after the echo (or instead of it) call exit 1
(or use a helper like die) so the generate-components.sh run stops immediately
when crd_pattern yields no CRDs; reference the echo line and the crd_pattern
variable to locate where to replace the warning-only behavior with an exit.
- Line 101: Summary: roleRef.name is hardcoded with the kustomize prefix; use
the short role name so kustomize can apply namePrefix. Fix: in the source
manifests (manager-rbac.yaml and manager-rolebinding.yaml) remove the hardcoded
"trustyai-service-operator-" prefix from the ClusterRole name and from
roleRef.name so both use the short role identifier (e.g., "manager-role" or the
component-local short name) and rely on namePrefix: trustyai-service-operator-
to add the prefix; ensure ClusterRole (kind: ClusterRole) name and roleRef.name
match exactly after this change and re-run the provided verification script to
confirm no mismatches.
---
Nitpick comments:
In `@hack/generate-components.sh`:
- Line 16: RBAC_DIR is defined but not used; update the delete invocation that
currently bypasses RBAC_DIR (the delete path around the code referenced at line
~117) to use the RBAC_DIR variable instead of the hardcoded "${CONFIG_DIR}/rbac"
(or any literal path), e.g. replace the hardcoded deletion target with
"${RBAC_DIR}" so the script consistently uses the RBAC_DIR variable (ensure any
surrounding quotes and variable expansions match existing usage).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/smoke.yamlcontrollers/evalhub/rbac_manifests_test.gocontrollers/evalhub/suite_test.gocontrollers/gorch/suite_test.gocontrollers/lmes/lmevaljob_controller_suite_test.gocontrollers/nemo_guardrails/suite_test.gocontrollers/tas/suite_test.gohack/generate-components.sh
|
@RobGeada: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR modifies the kustomize layout of the TrustyAI operator into a number of separate, modular components. The vast majority of the new lines introduced by this PR are simply from copying the generated manifests into component directories- the only real net new work is inside
/hackand inside theMakefile.Manifest generation is entirely automatic - first,
controller-gencreates manifests as normal. Then,components-generatesplits the manifests into subcomponents according to patterns defined inside ofhack/generate-components.sh. Then, overlays can choose which subcomponents of TrustyAI to enable by selectively including components. Each component is standalone, containing all necessary files for the controller to operate with its intended functionality.The
manifest-genmake target now accepts anOVERLAYargument to choose which overlay you wish to build a bundle for. If noOVERLAYis specified, then theodhbundle is built by default.To build a test bundle from this PR, fetch the branch, then run:
make manifest-gen NAMESPACE=redhat-ods-applications KUSTOMIZE=kustomizeSummary by CodeRabbit
New Features
Enhancements
Chores