feat(operator): expose structured triage actions - #108
Conversation
📝 WalkthroughWalkthroughThe triage API now supports multiple actions, lists actions from Applications, validates and normalizes action input, writes action arrays to TriageRuns, and parses per-action statuses while preserving legacy result fields. ChangesMulti-action triage runs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TriageRunRequest
participant normalizeActions
participant TriageRun
Caller->>TriageRunRequest: provide Actions
TriageRunRequest->>normalizeActions: validate and normalize action references
normalizeActions->>TriageRun: write spec.actions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
amwarrier
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/operator/triage.go (2)
187-193: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for the operator before submitting the v2 CR.
Lines 187-193 submit the v2
TriageRunwithoutWaitForOperator. CallWaitForOperatorbefore the submission and add a test that proves the wait completes first.As per coding guidelines,
pkg/operator/**/*.gorequiresWait for the operator webhook CA bundle to become non-empty before applying the v2 CR; do not skip WaitForOperator readiness handling.🤖 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 `@pkg/operator/triage.go` around lines 187 - 193, Update the v2 TriageRun submission flow around normalizedTriageActions and dynamicClient.Resource(...).Create to call WaitForOperator first, ensuring the operator webhook CA bundle is non-empty before creating the CR. Preserve the existing create behavior after readiness completes, and add a test verifying WaitForOperator finishes before the v2 TriageRun submission.Source: Coding guidelines
630-650: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse server-side apply for the v2
TriageRun.
createTriageRuncurrently sendsFieldManager: "wsm"inCreateOptions, which is still a normal create. For v2 CRs, callApply()withmetav1.ApplyOptions{FieldManager: "wsm", Force: true}, supply a concrete collision-safe object name, and ensure RBAC grantscreate/patchfor thetriagerunsresource.🤖 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 `@pkg/operator/triage.go` around lines 630 - 650, Update the v2 TriageRun creation flow in createTriageRun to use server-side Apply with metav1.ApplyOptions{FieldManager: "wsm", Force: true} instead of CreateOptions. Provide a concrete collision-safe metadata.name rather than relying on generateName, and update RBAC for the triageruns resource to grant both create and patch verbs.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/operator/triage_test.go (2)
62-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain a legacy
Actioncreation test.Lines 62-84 replace singular input coverage with an
Actionstest. Add a request withAction: "dependencies"and assert thatspec.actionscontains only"dependencies". This protects the public source-compatibility path.🤖 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 `@pkg/operator/triage_test.go` around lines 62 - 84, Add a separate test covering the legacy singular Action field in createTriageRun, using Action: "dependencies" and verifying the created object's spec.actions contains only "dependencies". Keep TestCreateTriageRunPreservesExplicitActions to cover the plural Actions input unchanged.
268-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the multi-action status path.
Lines 268-314 use one selected action and one action status. Add two
spec.actionsvalues and twostatus.actionStatusesentries with distinct results. AssertActionStatuses, flattenedResultsorder, and an empty legacyAction.🤖 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 `@pkg/operator/triage_test.go` around lines 268 - 314, Expand the triage test fixture to define two spec.actions and two status.actionStatuses with distinct result data, using the existing run objects and listTriageRuns flow. Update assertions to verify both ActionStatuses in order, flattened Results preserve the expected action/result order, and the legacy Action field remains empty.
🤖 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.
Outside diff comments:
In `@pkg/operator/triage.go`:
- Around line 187-193: Update the v2 TriageRun submission flow around
normalizedTriageActions and dynamicClient.Resource(...).Create to call
WaitForOperator first, ensuring the operator webhook CA bundle is non-empty
before creating the CR. Preserve the existing create behavior after readiness
completes, and add a test verifying WaitForOperator finishes before the v2
TriageRun submission.
- Around line 630-650: Update the v2 TriageRun creation flow in createTriageRun
to use server-side Apply with metav1.ApplyOptions{FieldManager: "wsm", Force:
true} instead of CreateOptions. Provide a concrete collision-safe metadata.name
rather than relying on generateName, and update RBAC for the triageruns resource
to grant both create and patch verbs.
---
Nitpick comments:
In `@pkg/operator/triage_test.go`:
- Around line 62-84: Add a separate test covering the legacy singular Action
field in createTriageRun, using Action: "dependencies" and verifying the created
object's spec.actions contains only "dependencies". Keep
TestCreateTriageRunPreservesExplicitActions to cover the plural Actions input
unchanged.
- Around line 268-314: Expand the triage test fixture to define two spec.actions
and two status.actionStatuses with distinct result data, using the existing run
objects and listTriageRuns flow. Update assertions to verify both ActionStatuses
in order, flattened Results preserve the expected action/result order, and the
legacy Action field remains empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a6107f1-c600-421d-9b98-f199b6a97041
📒 Files selected for processing (2)
pkg/operator/triage.gopkg/operator/triage_test.go
amwarrier
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/triage.go (1)
200-200: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse server-side apply for the v2
TriageRun.The test intercepts
"create"and requires that request to set the generated name. This confirms that the modified v2 creation path usesCreate, not server-side apply with field managerwsm.
pkg/operator/triage.go#L200-L200: apply the v2TriageRunwith server-side apply andFieldManager: "wsm". Rework name generation if necessary so the apply request has a stable object name.pkg/operator/triage_test.go#L24-L24: replace the create reactor with a patch reactor and assert the apply patch type and field manager.As per coding guidelines,
pkg/operator/**/*.gomust “Apply v2 CRs with server-side apply andFieldManager: "wsm".”🤖 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 `@pkg/operator/triage.go` at line 200, Update the v2 TriageRun creation flow in pkg/operator/triage.go at lines 200-200 to use server-side apply with FieldManager set to "wsm", ensuring the applied object has a stable generated name. Update pkg/operator/triage_test.go at lines 24-24 to intercept a patch instead of a create, and assert the apply patch type and "wsm" field manager.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 `@pkg/operator/triage.go`:
- Around line 514-529: Reject empty action selections and names: in
pkg/operator/triage.go lines 514-529, make the parser error for an empty action
array and for object or string names that are empty after trimming whitespace;
in lines 653-663, distinguish a non-nil empty Actions slice from omitted Actions
and reject the explicit empty selection while preserving omitted-action default
behavior. Add validation and parsing tests covering spec.actions as [], {}, "",
empty arrays, and whitespace-only names.
- Around line 286-288: Update the legacy action handling in the item-to-action
conversion path to capture and propagate the error returned by
unstructured.NestedString when reading “description” from actionMap. Return a
contextual error matching the array-shaped catalog behavior instead of silently
accepting an empty description.
---
Outside diff comments:
In `@pkg/operator/triage.go`:
- Line 200: Update the v2 TriageRun creation flow in pkg/operator/triage.go at
lines 200-200 to use server-side apply with FieldManager set to "wsm", ensuring
the applied object has a stable generated name. Update
pkg/operator/triage_test.go at lines 24-24 to intercept a patch instead of a
create, and assert the apply patch type and "wsm" field manager.
🪄 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: 5c6198ad-f8ba-4901-b5fa-adb4c5ed60e8
📒 Files selected for processing (2)
pkg/operator/triage.gopkg/operator/triage_test.go
|
|
||
| // Action is retained for source compatibility with clients of the initial | ||
| // single-action SDK. New callers should use Actions; setting both is invalid. | ||
| Action string `json:"action,omitempty"` |
There was a problem hiding this comment.
Does this need to be backwards compatible for something that we haven't implemented yet? I think it should just support a list of actions, that list can be of length 1. We don't need to support both a list and a single action
There was a problem hiding this comment.
my bad. removed Action
|
|
||
| // ListTriageActions returns the sorted action metadata declared by one | ||
| // Application. | ||
| func ListTriageActions(ctx context.Context, namespace, applicationName string) ([]TriageAction, error) { |
There was a problem hiding this comment.
This function isn't used anywhere in wsm? Is this for watchtower? You also have a function called listTriageActions (lowercase l) at line 222, that's super confusing. Which function is supposed to be here?
There was a problem hiding this comment.
renamed it to listTriageActionsWithClient
There was a problem hiding this comment.
ListTriageActions is invoked by watchtower when the UI check loads.
| ApplicationName string `json:"applicationName"` | ||
| Actions []TriageActionReference `json:"actions"` | ||
| // Action mirrors the selected action for legacy single-action consumers. | ||
| Action string `json:"action,omitempty"` |
There was a problem hiding this comment.
Same here, can remove singular Action.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/triage.go (1)
194-194: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd readiness wait before creating the v2 TriageRun.
The create path uses
FieldManager: "wsm", butCreateTriageRuncallsCreatedirectly without waiting for the operator. AddWaitForOperatorbefore the create call so server-side apply is applied only after the webhook CA bundle is ready, as required forpkg/operator/**/*.go.🤖 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 `@pkg/operator/triage.go` at line 194, In the v2 TriageRun creation flow surrounding normalizedTriageActions, call WaitForOperator before CreateTriageRun so the operator and webhook CA bundle are ready before the FieldManager "wsm" server-side apply occurs. Preserve the existing action normalization and create behavior after readiness succeeds.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.
Outside diff comments:
In `@pkg/operator/triage.go`:
- Line 194: In the v2 TriageRun creation flow surrounding
normalizedTriageActions, call WaitForOperator before CreateTriageRun so the
operator and webhook CA bundle are ready before the FieldManager "wsm"
server-side apply occurs. Preserve the existing action normalization and create
behavior after readiness succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4697a6d6-a202-4daa-843e-df3b12db6f63
📒 Files selected for processing (2)
pkg/operator/triage.gopkg/operator/triage_test.go
2 participants
Summary of Changes
This PR updates the TriageRun SDK for structured actions.
{name, description}objects fromApplication.spec.triage.actions{name}action referencesJira: N/A
Test Plan
go test ./pkg/operator -run Triage -count=1go vet ./...golangci-lint run --timeout=5m --concurrency=4 --max-same-issues=20go build -tags containers_image_openpgp ./cmd/wsmRequirements
make lintequivalent commands passed)make fmtleaves no diff)go.mod/go.sumremain unchanged by this PRSummary by CodeRabbit
New Features
Bug Fixes