HYPERFLEET-1291 - feat: add domain-specific custom CEL functions#229
Open
rafabene wants to merge 1 commit into
Open
HYPERFLEET-1291 - feat: add domain-specific custom CEL functions#229rafabene wants to merge 1 commit into
rafabene wants to merge 1 commit into
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds domain-specific CEL helpers for condition status, condition age, stability checks, status feedback lookup, and tri-state mapping in the evaluator. Documentation was updated to show helper-based forms and before/after examples. Tests and dry-run task data were extended to cover normal and malformed inputs. The golangci-lint bingo module’s Go directive was bumped to 1.26.0. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TaskConfig
participant CELEvaluator
participant customCELFunctions
participant findCondition
TaskConfig->>CELEvaluator: EvaluateSafe(helper expression, conditions)
CELEvaluator->>customCELFunctions: invoke registered helper
customCELFunctions->>findCondition: resolve matching condition
customCELFunctions-->>CELEvaluator: derived string/bool/age value
CELEvaluator-->>TaskConfig: computed payload value
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 6
🧹 Nitpick comments (2)
internal/criteria/cel_evaluator.go (2)
161-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate transition-time parsing logic between
conditionAgeandstableFor.Both bindings repeat the same
findCondition→cond["last_transition_time"].(string)→time.Parse(time.RFC3339, ...)→time.Since(t).Seconds()sequence. Extract a shared helper (e.g.conditionAgeSeconds(cond map[string]interface{}) (int64, bool)) to avoid drift between the two implementations.♻️ Proposed extraction
func transitionAgeSeconds(cond map[string]interface{}) (int64, bool) { transitionTime, ok := cond["last_transition_time"].(string) if !ok { return 0, false } t, err := time.Parse(time.RFC3339, transitionTime) if err != nil { return 0, false } return int64(time.Since(t).Seconds()), true }Then in
conditionAge:- transitionTime, ok := cond["last_transition_time"].(string) - if !ok { - return types.Int(-1) - } - t, err := time.Parse(time.RFC3339, transitionTime) - if err != nil { - return types.Int(-1) - } - return types.Int(int64(time.Since(t).Seconds())) + age, ok := transitionAgeSeconds(cond) + if !ok { + return types.Int(-1) + } + return types.Int(age)🤖 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/criteria/cel_evaluator.go` around lines 161 - 227, Both conditionAge and stableFor duplicate the same last_transition_time parsing and age calculation logic, so extract that sequence into a shared helper in cel_evaluator.go (for example, a transitionAgeSeconds/conditionAgeSeconds helper used by both bindings). Update the conditionAge and stableFor implementations to call the helper after findCondition, keeping their existing return behavior while centralizing the RFC3339 parsing and time.Since math.Source: Path instructions
161-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated field-name string literals across all new bindings.
"last_transition_time","status","type","values","name","fieldValue","string"are hardcoded in multiple places. Extracting these into named constants would reduce typo risk if the API contract fields ever get touched.🤖 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/criteria/cel_evaluator.go` around lines 161 - 282, Extract the repeated CEL field-name string literals used by conditionAge, stableFor, statusFeedbackValue, and triState into shared named constants to avoid typos and keep the API contract centralized. Update the bindings in cel_evaluator.go to reference those constants instead of hardcoded strings, especially for the keys used by findCondition, unwrapCELList, and the feedback map lookup logic. Keep the behavior unchanged while making the field access symbols easy to maintain.Source: Path instructions
🤖 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/adapter-authoring-guide.md`:
- Around line 1610-1616: The verbose fallback example is inconsistent with the
behavior of conditionStatus(), which returns "Unknown" when no matching
condition is found. Update the fallback in the illustrated filter/ternary
expression to use the same default as conditionStatus(conditions, "Reconciled"),
so the before/after example stays equivalent and teaches the correct
absent-condition behavior.
- Around line 1683-1686: The preferred example for statusFeedbackValue() still
hard-dereferences resources.namespace0.statusFeedback, so it can fail before the
helper handles missing feedback. Update the example around statusFeedbackValue()
to guard the namespace0 lookup first, or use the safer verbose pattern shown
below, so the sample only accesses statusFeedback when namespace0 exists.
In `@docs/conventions/cel.md`:
- Around line 24-26: Clarify the stableFor() boundary semantics in the CEL
conventions docs so they match the implementation: stableFor(conditions, type,
seconds) should be described as returning true when conditionStatus is "True"
and conditionAge is greater than or equal to the threshold, not strictly
greater. Update the surrounding prose and any example wording in the docs
section for conditionStatus, conditionAge, and stableFor so the 300-second
boundary is unambiguous.
In `@internal/criteria/cel_evaluator_test.go`:
- Around line 375-554: Add tests in TestCELEvaluatorDomainFunctions to cover the
fallback branches that are currently untested. Exercise conditionAge and
stableFor with malformed last_transition_time values so time.Parse fails and the
functions return their fallback result, and add statusFeedbackValue cases where
values is not a list, an entry is not a map, or fieldValue.string is missing.
Use the existing evaluator, NewEvaluationContext, and EvaluateSafe patterns to
assert the expected fallback outputs for each malformed input.
In `@internal/criteria/cel_evaluator.go`:
- Around line 216-224: The stableFor check in cel_evaluator.go currently treats
the condition as true when age is equal to the threshold, but the documented
contract for stableFor says it must exceed the threshold. Update the comparison
in the stableFor evaluation path inside the CEL evaluator logic so it uses a
strict greater-than instead of greater-than-or-equal, keeping the behavior
aligned with the docs and the existing transitionTime parsing flow.
In `@test/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yaml`:
- Around line 468-470: The is_reconciled_stable CEL expression is checking the
wrong condition name, so update the stableFor call in the
dryrun-cel-showcase-task-config payload to use Reconciled instead of
Provisioned. Keep the change scoped to the is_reconciled_stable entry so it
exercises the reconciled-condition helper path as intended and matches the
related stableFor(conditions, "Reconciled", ...) behavior.
---
Nitpick comments:
In `@internal/criteria/cel_evaluator.go`:
- Around line 161-227: Both conditionAge and stableFor duplicate the same
last_transition_time parsing and age calculation logic, so extract that sequence
into a shared helper in cel_evaluator.go (for example, a
transitionAgeSeconds/conditionAgeSeconds helper used by both bindings). Update
the conditionAge and stableFor implementations to call the helper after
findCondition, keeping their existing return behavior while centralizing the
RFC3339 parsing and time.Since math.
- Around line 161-282: Extract the repeated CEL field-name string literals used
by conditionAge, stableFor, statusFeedbackValue, and triState into shared named
constants to avoid typos and keep the API contract centralized. Update the
bindings in cel_evaluator.go to reference those constants instead of hardcoded
strings, especially for the keys used by findCondition, unwrapCELList, and the
feedback map lookup logic. Keep the behavior unchanged while making the field
access symbols easy to maintain.
🪄 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: CHILL
Plan: Enterprise
Run ID: 29d445a0-fc10-435b-ae9c-cd53710b4120
📒 Files selected for processing (7)
.bingo/golangci-lint.moddocs/adapter-authoring-guide.mddocs/conventions/cel.mdinternal/criteria/cel_evaluator.gointernal/criteria/cel_evaluator_test.gotest/testdata/dryrun/cel-showcase/dryrun-cel-showcase-api-responses.jsontest/testdata/dryrun/cel-showcase/dryrun-cel-showcase-task-config.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 592 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
Add 5 CEL helper functions to reduce adapter config verbosity: - conditionStatus(conditions, type) → extracts status from conditions list - conditionAge(conditions, type) → elapsed seconds since last_transition_time - stableFor(conditions, type, seconds) → stability window check - statusFeedbackValue(statusFeedback, name) → Maestro feedback extraction - triState(trueCond, falseCond) → maps two booleans to "True"/"False"/"Unknown" Also bumps .bingo/golangci-lint.mod to go 1.26.0 (HYPERFLEET-1309).
rafabene
force-pushed
the
HYPERFLEET-1291-domain-specific-cel-functions
branch
from
9cd3e2d to
4efa60a
Compare
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
1 participant
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.
Summary
conditionStatus(conditions, type)— replaces the double-filter pattern (~12x per ROSA config)conditionAge(conditions, type)— replacestimestamp(now()) - timestamp(...)chainsstableFor(conditions, type, seconds)— combines conditionStatus + conditionAge into a single stability-window checkstatusFeedbackValue(statusFeedback, name)— replaces 4-step filter chain for Maestro ManifestWork feedbacktriState(trueCond, falseCond)— replaces nested ternaries for "True"/"False"/"Unknown" mappingdocs/conventions/cel.mdwith signatures, descriptions, and before/after examplesdocs/adapter-authoring-guide.mdAppendix A (CEL Quick Reference) and adds simplified version of time-based stability precondition pattern.bingo/golangci-lint.modtogo 1.26.0to align with HYPERFLEET-1309Test plan
make test— all 15 packages passmake lint— 0 issues