HYPERFLEET-876 - feat: add when condition to post-actions#122
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change adds conditional execution for post-actions via a new optional Sequence Diagram(s)sequenceDiagram
actor Client as PostActionExecutor
participant When as PostAction.When
participant CEL as CEL Evaluator
participant ExecCtx as Exec Context
participant API as API Client
participant Result as PostActionResult
Client->>When: Is When set?
alt When set
Client->>ExecCtx: Build CEL variables
Client->>CEL: Create evaluator with variables
alt evaluator creation fails
CEL-->>Result: Return ExecutorError
else evaluator created
Client->>CEL: Evaluate Expression
alt evaluation error
CEL-->>Result: Return ExecutorError
else evaluation succeeds
alt expression true
Client->>API: Execute API call / perform action
API-->>Result: Return success/status
else expression false
Client-->>Result: Mark Skipped, StatusSkipped, set SkipReason
end
end
end
else When not set
Client->>API: Execute API call / perform action
API-->>Result: Return success/status
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @cristianoveiga. Thanks for your PR. I'm waiting for a openshift-hyperfleet member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
cristianoveiga
force-pushed
the
feat/HYPERFLEET-876-post-action-when
branch
2 times, most recently
from
6e6c28a to
96dda70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/configloader/types.go`:
- Around line 406-411: PostActionWhen.Expression is documented as required but
lacks a validation tag, so add a validation requirement to the Expression field
(e.g., add validate:"required" alongside the existing yaml tag) on the
PostActionWhen struct to ensure config validation fails fast; update any
validation code that reads struct tags if necessary so the
PostActionWhen.Expression field is enforced during config parsing/validation.
In `@internal/executor/post_action_executor_test.go`:
- Around line 730-737: The test currently only checks API-call side effects
(assert.Equal(t, tt.wantAPICall, result.APICallMade)) but doesn't assert skip
semantics; after require.NoError(t, err) add assertions that verify
result.Skipped matches the expected skip outcome (e.g. assert.Equal(t,
tt.wantSkipped, result.Skipped) or assert.False/True for the false `when` cases)
and, where applicable, assert the SkipReason (e.g. assert.Equal(t,
tt.wantSkipReason, result.SkipReason)) so the test enforces "skipped, not
failed" semantics in the same test block that checks APICallMade and error
state.
In `@internal/executor/post_action_executor.go`:
- Around line 260-264: The skip path in PostActionExecutor currently sets
result.Skipped = true but leaves result.Status at the default (StatusSuccess)
and logs unconditionally, which can make callers treat skipped actions as
successes; update the branch that checks celResult.Matched to also set
result.Status = StatusSkipped (or the appropriate skipped enum/constant used by
the executor), change the log call to conditional logging based on
result.Skipped (e.g., log a skip message when result.Skipped is true instead of
always Infof), and continue returning result, nil so callers can accurately
report skipped actions; reference symbols: celResult.Matched, result.Skipped,
result.Status, action.When.Expression, and the logging call pae.log.Infof.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c8f3230b-1126-4eda-a374-56974567f558
📒 Files selected for processing (3)
internal/configloader/types.gointernal/executor/post_action_executor.gointernal/executor/post_action_executor_test.go
Extends the adapter framework to support a `when` condition on post-actions. If the CEL expression evaluates to false, the post-action is skipped (not failed). Also fixes two misleading nolint:govet fieldalignment comments — both suppress genuine false positives where no field reordering can reduce the struct size. Jira: https://redhat.atlassian.net/browse/HYPERFLEET-876 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cristianoveiga
force-pushed
the
feat/HYPERFLEET-876-post-action-when
branch
from
96dda70 to
01871a7
Compare
…-action Skip status reporting when the adapter did no work (resourcesSkipped=true) to avoid overwriting a previously reported status with stale data. Jira: https://redhat.atlassian.net/browse/HYPERFLEET-876 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
/ok-to-test |
cristianoveiga
changed the title
Move the fieldalignment rationale into the doc comment and shorten the nolint directive to stay within the 120-char lll limit. Co-Authored-By: Claude <noreply@anthropic.com>
cristianoveiga
force-pushed
the
feat/HYPERFLEET-876-post-action-when
branch
from
135cc5a to
61a7dfb
Compare
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 `@internal/executor/post_action_executor.go`:
- Around line 253-263: The PostActionResult (result) isn't updated when CEL
evaluation fails so callers see StatusSuccess; in the error branches around
criteria.NewEvaluator, evaluator.EvaluateCEL, and celResult.HasError set
result.Status to a failure state (e.g., StatusFailed) and populate result.Error
with the underlying error (err or celResult.Error) before returning
NewExecutorError; update the error-returning paths in
PostActionExecutor.Execute/ExecuteAll so ExecuteAll appends a result that
accurately reflects the failure.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ece2abe2-af54-4ddb-b128-b900b864e554
📒 Files selected for processing (7)
charts/examples/maestro-two-resources/adapter-task-config.yamlcharts/examples/maestro/adapter-task-config.yamlconfigs/adapter-task-config-template.yamlinternal/configloader/types.gointernal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/types.go
Reorder struct fields in TestPostActionWhenCondition to fix the govet/fieldalignment violation: move the `when` pointer field before the `name` string field so pointer bytes reduce from 24 to 16. Signed-off-by: Cristiano Veiga <cveiga@redhat.com> Co-Authored-By: Claude <noreply@anthropic.com>
cristianoveiga
force-pushed
the
feat/HYPERFLEET-876-post-action-when
branch
from
f6a125e to
46f2b14
Compare
|
/ok-to-test |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 55894fa
into
openshift-hyperfleet:main
3 participants
Summary
Extends the adapter framework to support a
whencondition on post-actions. If thewhenexpression evaluates tofalse, the post-action is skipped. This allows adapters to conditionally skip status reporting or other post-actions based on execution state (e.g. skip reporting when resources were skipped).Jira: https://redhat.atlassian.net/browse/HYPERFLEET-876
Changes
internal/configloader/types.go: addWhenfield toPostActionConfiginternal/executor/post_action_executor.go: evaluatewhenCEL expression before executing; skip if falseinternal/executor/post_action_executor_test.go: test coverage for when-gated post-actions (skip/execute branches)Test plan
go test ./...)when: expression: "!adapter.resourcesSkipped"skips correctly when resources are skipped🤖 Generated with Claude Code