feat(api): Define TriageRun CRD - #304
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdded the namespaced ChangesTriageRun v2 resource
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/crdinstaller/compose_test.go (1)
72-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse the configured Ginkgo/Gomega test suite.
The added TriageRun coverage remains in a
testing.Ttest. Move this coverage to the Ginkgo/Gomega suite configured throughsuite_test.go. Before completion, runmake lintandmake test.As per coding guidelines, “Use Ginkgo/Gomega for tests; test suites are configured through
suite_test.gofiles and envtest,” and “Run bothmake lintandmake testbefore considering a task complete.”Also applies to: 107-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/crdinstaller/compose_test.go` around lines 72 - 97, Move the CRD composition coverage around the existing test function into the configured Ginkgo/Gomega suite from suite_test.go, replacing testing.T assertions with the suite’s established Ginkgo/Gomega style while preserving all CRD, annotation, webhook service, and expected-name checks. Apply the same conversion to the related coverage at the referenced additional location, then run make lint and make test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/crd/bases/apps.wandb.com_triageruns.yaml`:
- Around line 43-62: The root TriageRun schema does not require spec, allowing
validation rules to be bypassed. Update the Spec field markers in
api/v2/triagerun_types.go to make spec required, then regenerate both
config/crd/bases/apps.wandb.com_triageruns.yaml (anchor, lines 43-62) and
internal/crdinstaller/crds/operator/apps.wandb.com_triageruns.yaml (sibling,
lines 43-62) using make manifests generate sync-crd-embed; do not edit either
CRD manifest manually.
---
Nitpick comments:
In `@internal/crdinstaller/compose_test.go`:
- Around line 72-97: Move the CRD composition coverage around the existing test
function into the configured Ginkgo/Gomega suite from suite_test.go, replacing
testing.T assertions with the suite’s established Ginkgo/Gomega style while
preserving all CRD, annotation, webhook service, and expected-name checks. Apply
the same conversion to the related coverage at the referenced additional
location, then run make lint and make test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f5f6913-b1b4-4c89-991b-547a7a12b5ad
📒 Files selected for processing (15)
PROJECTapi/v2/triagerun_types.goapi/v2/zz_generated.deepcopy.goconfig/crd/bases/apps.wandb.com_triageruns.yamlconfig/crd/bases/kustomization.yamlconfig/dev-common/delete-triageruns-crd.yamlconfig/dev-common/kustomization.yamlconfig/rbac/kustomization.yamlconfig/rbac/triagerun_admin_role.yamlconfig/rbac/triagerun_editor_role.yamlconfig/rbac/triagerun_viewer_role.yamlconfig/samples/apps_v2_triagerun.yamlconfig/samples/kustomization.yamlinternal/crdinstaller/compose_test.gointernal/crdinstaller/crds/operator/apps.wandb.com_triageruns.yaml
amwarrier
changed the title
a79a269 to
a093d55
Compare
a093d55 to
373408f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
config/rbac/triagerun_admin_role.yaml (1)
14-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse an explicit verb allowlist for this admin role.
triagerun-admin-rolegrantsverbs: ['*']to external cluster admins onapps.wandb.com/triageruns, so every current and future verb is available there. Replace the wildcard with the required verbs, such asget,list,watch,create,update,patch,delete, anddeletecollection, while leavingtriageruns/statusread-only.Proposed change
- - '*' + - get + - list + - watch + - create + - update + - patch + - delete + - deletecollection🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/rbac/triagerun_admin_role.yaml` around lines 14 - 19, Update the triagerun-admin-role rule for apps.wandb.com/triageruns to replace the wildcard verb with an explicit allowlist of get, list, watch, create, update, patch, delete, and deletecollection; preserve the separate triageruns/status rule as read-only.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/triage-runs.md`:
- Around line 363-366: Update the implementation-slice table entry for
operator#304 to remove or correctly label Application.spec.triage according to
the PR’s actual delivered scope, and revise the related availability statement
around the triage behavior section so it does not claim that
Application.spec.triage declarations or reconciliation are already provided by
this change.
- Around line 84-86: Update the sequence diagram so the Job-to-API interaction
represents only the Job completion event/status, removing the implication that
JSONL stdout is sent as a Job payload. Preserve the existing C->>API: Read pod
logs step as the path through which JSONL results are retrieved.
---
Nitpick comments:
In `@config/rbac/triagerun_admin_role.yaml`:
- Around line 14-19: Update the triagerun-admin-role rule for
apps.wandb.com/triageruns to replace the wildcard verb with an explicit
allowlist of get, list, watch, create, update, patch, delete, and
deletecollection; preserve the separate triageruns/status rule as read-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91b0606a-8414-4a43-afcb-f684eb969278
📒 Files selected for processing (16)
PROJECTapi/v2/triagerun_types.goapi/v2/zz_generated.deepcopy.goconfig/crd/bases/apps.wandb.com_triageruns.yamlconfig/crd/bases/kustomization.yamlconfig/dev-common/delete-triageruns-crd.yamlconfig/dev-common/kustomization.yamlconfig/rbac/kustomization.yamlconfig/rbac/triagerun_admin_role.yamlconfig/rbac/triagerun_editor_role.yamlconfig/rbac/triagerun_viewer_role.yamlconfig/samples/apps_v2_triagerun.yamlconfig/samples/kustomization.yamldocs/design/triage-runs.mdinternal/crdinstaller/compose_test.gointernal/crdinstaller/crds/operator/apps.wandb.com_triageruns.yaml
🚧 Files skipped from review as they are similar to previous changes (14)
- PROJECT
- config/samples/kustomization.yaml
- config/samples/apps_v2_triagerun.yaml
- config/dev-common/kustomization.yaml
- config/dev-common/delete-triageruns-crd.yaml
- config/rbac/triagerun_editor_role.yaml
- config/rbac/kustomization.yaml
- internal/crdinstaller/compose_test.go
- config/crd/bases/kustomization.yaml
- config/crd/bases/apps.wandb.com_triageruns.yaml
- config/rbac/triagerun_viewer_role.yaml
- api/v2/triagerun_types.go
- internal/crdinstaller/crds/operator/apps.wandb.com_triageruns.yaml
- api/v2/zz_generated.deepcopy.go
| Job-->>API: JSONL results on stdout and exit status | ||
| API-->>C: Job completion event | ||
| C->>API: Read pod logs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the Pod log path in the sequence diagram.
Job does not send container stdout as a Job payload to the Kubernetes API. The diagnostic container writes JSONL to stdout. Kubernetes exposes that output through Pod logs. The controller reads those logs after the Job completion event.
Update the diagram so this step shows Job completion only. Keep the later C->>API: Read pod logs step as the JSONL transfer path.
Proposed change
- Job-->>API: JSONL results on stdout and exit status
+ Job-->>API: Job completion status📝 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.
| Job-->>API: JSONL results on stdout and exit status | |
| API-->>C: Job completion event | |
| C->>API: Read pod logs | |
| Job-->>API: Job completion status | |
| API-->>C: Job completion event | |
| C->>API: Read pod logs |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/triage-runs.md` around lines 84 - 86, Update the sequence diagram
so the Job-to-API interaction represents only the Job completion event/status,
removing the implication that JSONL stdout is sent as a Job payload. Preserve
the existing C->>API: Read pod logs step as the path through which JSONL results
are retrieved.
| | Repository and PR | Responsibility | | ||
| | --- | --- | | ||
| | [operator#304](https://github.com/wandb/operator/pull/304) | `TriageRun` API, `Application.spec.triage`, CRD, generated API roles, and embedded CRD packaging | | ||
| | [operator#305](https://github.com/wandb/operator/pull/305) | `TriageRun` reconciler, Job construction, log parsing, status, and controller RBAC | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the implementation-slice table with the PR scope.
The PR objective states that this change does not add Application.spec.triage declarations or reconciliation. Line 365 assigns Application.spec.triage to operator#304, and Lines 121-124 present that behavior as already available.
Update the row to describe only delivered work, or mark Application.spec.triage as follow-up work with the correct PR. Otherwise, the document overstates the API contract delivered by this change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/triage-runs.md` around lines 363 - 366, Update the
implementation-slice table entry for operator#304 to remove or correctly label
Application.spec.triage according to the PR’s actual delivered scope, and revise
the related availability statement around the triage behavior section so it does
not claim that Application.spec.triage declarations or reconciliation are
already provided by this change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v2/triagerun_types.go`:
- Around line 52-54: Update the ApplicationRef.Name validation markers in the
triage run API type to enforce a Kubernetes DNS subdomain: add the DNS subdomain
regex and a maximum length of 253 while retaining the minimum length
requirement. Then regenerate the related deepcopy code, CRD YAML, and embedded
CRDs using the repository’s standard generation workflow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5faa0531-138f-435d-b7b3-f9ab5fabe3e6
📒 Files selected for processing (15)
PROJECTapi/v2/triagerun_types.goapi/v2/zz_generated.deepcopy.goconfig/crd/bases/apps.wandb.com_triageruns.yamlconfig/crd/bases/kustomization.yamlconfig/dev-common/delete-triageruns-crd.yamlconfig/dev-common/kustomization.yamlconfig/rbac/kustomization.yamlconfig/rbac/triagerun_admin_role.yamlconfig/rbac/triagerun_editor_role.yamlconfig/rbac/triagerun_viewer_role.yamlconfig/samples/apps_v2_triagerun.yamlconfig/samples/kustomization.yamlinternal/crdinstaller/compose_test.gointernal/crdinstaller/crds/operator/apps.wandb.com_triageruns.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- config/rbac/triagerun_editor_role.yaml
- PROJECT
- config/rbac/triagerun_viewer_role.yaml
- config/dev-common/delete-triageruns-crd.yaml
- config/samples/apps_v2_triagerun.yaml
- config/crd/bases/kustomization.yaml
- config/samples/kustomization.yaml
- config/crd/bases/apps.wandb.com_triageruns.yaml
- config/dev-common/kustomization.yaml
- config/rbac/kustomization.yaml
- internal/crdinstaller/compose_test.go
- api/v2/zz_generated.deepcopy.go
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
1 participant
Summary
Define the namespaced
apps.wandb.com/v2TriageRunAPI for immutable, on-demand diagnostic executions. A run references oneApplicationand selects one or more actions by object reference:spec.actionsuses Kubernetes map-list semantics keyed byname, so selections are structured and duplicate action names are rejected. Descriptions and execution details remain on the referencedApplication; the immutable run stores only the selected names.Status contains aggregate run state plus map-style per-action status, including the owned Job reference, resolved execution snapshot, timestamps, summary, and structured check results.
This PR contains the API, generated CRD/deep-copy artifacts, sample manifest, RBAC scaffolding, and embedded installer CRD. Reconciliation is added by the next PR in the stack.
Validation
go test ./api/v2 ./internal/crdinstaller/...x-kubernetes-list-type: mapwithnameas the list-map key.golang.org/x/textis upgraded tov0.39.0to clear CVE-2026-56852 from the image scan.Stack
Summary by CodeRabbit
New Features
TriageRunv2 resource for configuring and tracking triage actions.TriageRunmanifest.Developer Experience