HYPERFLEET-537 - feat: refactor condition_rules to use CEL expressions#72
Draft
rafabene wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Draft
HYPERFLEET-537 - feat: refactor condition_rules to use CEL expressions#72rafabene wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
rafabene wants to merge 1 commit intoopenshift-hyperfleet: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 |
|
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:
WalkthroughReplaces per-status max-age fields with a CEL-driven conditions model. Adds ConditionRule and Conditions types to config and removes MaxAgeNotReady/MaxAgeReady. Introduces CompileConditions and CompiledConditions (CEL compilation/evaluation) and updates DecisionEngine to accept compiled conditions. Adds CEL runtime (custom functions), resource.ToMap conversion, and payload evaluation changes to use CEL results. Helm charts, configs, docs, templates, and many tests are updated to the conditions-based configuration. Evaluation uses EvalReferenceTime and EvalMaxAge (first-matching rule’s max_age, fallback to minimum max_age) to compute next-event times. Sequence Diagram(s)sequenceDiagram
participant YAML as Configuration (YAML)
participant Loader as Config Loader/Validator
participant Compiler as CEL Compiler
participant Engine as DecisionEngine
participant Resource as Resource
participant Compiled as CompiledConditions
YAML->>Loader: Load conditions.reference_time + rules[]
Loader->>Loader: Validate reference_time and rules
Loader->>Compiler: CompileConditions(conditions)
Compiler-->>Loader: *CompiledConditions*
Loader->>Engine: NewDecisionEngine(*CompiledConditions*)
Resource->>Resource: ToMap(resource)
Resource->>Engine: Evaluate(resource, now)
Engine->>Compiled: EvalReferenceTime(resourceMap)
Compiled-->>Engine: referenceTime (or fallback)
Engine->>Compiled: EvalMaxAge(resourceMap)
Compiled-->>Engine: selected maxAge (first-match or min fallback)
Engine->>Engine: compute nextEvent = referenceTime + maxAge
Engine-->>Resource: Decision (nextEventTime)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sentinel/sentinel.go (1)
68-69: Consider logging a summary instead of the full rules slice.Logging
rules=%vwill output the entire slice structure, which could be verbose if many rules are configured. Consider logging just the rule count or a condensed format for cleaner startup logs.♻️ Optional: Log rule count instead of full slice
- s.logger.Infof(ctx, "Starting sentinel resource_type=%s poll_interval=%s condition_type=%s rules=%v", - s.config.ResourceType, s.config.PollInterval, s.config.ConditionRules.ConditionType, s.config.ConditionRules.Rules) + s.logger.Infof(ctx, "Starting sentinel resource_type=%s poll_interval=%s condition_type=%s rule_count=%d", + s.config.ResourceType, s.config.PollInterval, s.config.ConditionRules.ConditionType, len(s.config.ConditionRules.Rules))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel.go` around lines 68 - 69, The startup log currently prints the entire rules slice via s.logger.Infof(...) with s.config.ConditionRules.Rules which can be very verbose; change the message to log a concise summary (e.g., the rule count and/or a condensed representation) instead of the full slice. Update the call to s.logger.Infof to include len(s.config.ConditionRules.Rules) (and optionally a short sample or joined IDs) and remove printing the full Rules slice so the log line remains compact while still showing ResourceType, PollInterval, ConditionType and a rules_count field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/sentinel/sentinel.go`:
- Around line 68-69: The startup log currently prints the entire rules slice via
s.logger.Infof(...) with s.config.ConditionRules.Rules which can be very
verbose; change the message to log a concise summary (e.g., the rule count
and/or a condensed representation) instead of the full slice. Update the call to
s.logger.Infof to include len(s.config.ConditionRules.Rules) (and optionally a
short sample or joined IDs) and remove printing the full Rules slice so the log
line remains compact while still showing ResourceType, PollInterval,
ConditionType and a rules_count field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 289b77d2-38f2-4417-9824-bc3fac2228d6
📒 Files selected for processing (20)
MakefileREADME.mdcharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/sentinel-operator-guide.mdinternal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/valid-complete.yamlinternal/engine/decision.gointernal/engine/decision_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.go
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
07d9f7b to
11b82be
Compare
rafabene
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/integration/integration_test.go (1)
149-170:⚠️ Potential issue | 🟠 MajorThese integration fixtures still short-circuit on generation mismatch.
DecisionEngine.Evaluatechecksr.Generation > r.Status.ObservedGenerationbefore any CEL rule evaluation oninternal/engine/decision.goLine 75, andinternal/resource/convert.gomodelsstatus.observed_generationseparately from the condition-level values. These setups still usecreateMockClusterWithLabels, which only setsobserved_generationinside each condition, so the resources publish forReasonGenerationChanged, not because the newcondition_ruleslogic matched.Suggested fixture fix in
createMockClusterWithLabels"status": map[string]interface{}{ + "observed_generation": observedGeneration, "conditions": []map[string]interface{}{ readyCondition, {Also applies to: 275-296, 396-416, 509-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/integration_test.go` around lines 149 - 170, The tests are short-circuiting on generation mismatch because DecisionEngine.Evaluate checks r.Generation > r.Status.ObservedGeneration before evaluating CEL rules; update the fixtures (createMockClusterWithLabels and the other occurrences mentioned) to set the top-level resource.Generation and resource.Status.ObservedGeneration consistently (e.g., set Status.ObservedGeneration = Generation) instead of only populating per-condition observed_generation so the generation check does not trigger ReasonGenerationChanged and the new condition_rules logic runs.internal/config/config_test.go (1)
277-295:⚠️ Potential issue | 🟠 MajorSet
ResourceTypebefore exercising the duration checks.Line 292 starts from
NewSentinelConfig(), which leavesResourceTypeempty.Validate()can fail onresource_type is requiredbefore it ever reaches the negative/zero duration branches, so these cases will keep passing even if the duration validation regresses.Suggested fix
t.Run(tt.name, func(t *testing.T) { cfg := NewSentinelConfig() + cfg.ResourceType = "clusters" cfg.HyperFleetAPI.Endpoint = "http://api.example.com" cfg.MessageData = map[string]interface{}{"id": "resource.id"} tt.modifier(cfg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 277 - 295, The tests call NewSentinelConfig() and then exercise duration validation (via Validate()) but never set SentinelConfig.ResourceType, so Validate() may return "resource_type is required" before negative/zero MaxAge cases run; update the test setup inside the loop (after cfg := NewSentinelConfig() and before tt.modifier(cfg)) to assign a valid cfg.ResourceType (e.g., a non-empty string) so the duration checks in Validate() for Rules[0].MaxAge are actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/README.md`:
- Around line 83-84: Update the README table to use the actual chart keys:
replace occurrences of config.conditionRules.conditionType and
config.conditionRules.rules with the chart's config.conditions keys (e.g.,
config.conditions.referenceTime and config.conditions.rules) so the documented
values match what the chart consumes; ensure the description and default value
in the table reflect the semantics of config.conditions.referenceTime and
config.conditions.rules.
In `@charts/templates/configmap.yaml`:
- Around line 15-18: The template emits rule names as unquoted scalars which can
break YAML for names containing special chars; update the rules loop in the
template that iterates over .Values.config.conditions.rules so the name field is
emitted as a quoted string (e.g. apply the quote filter to .name) instead of a
plain scalar, keeping expression and max_age as-is.
In `@internal/config/config.go`:
- Around line 158-175: The Validate() function currently only rejects empty
strings but allows whitespace-only values; update validation for
c.Conditions.ReferenceTime and for each rule in c.Conditions.Rules to trim
whitespace (e.g., strings.TrimSpace) before checking emptiness so that values
like " " are treated as invalid; keep the same error messages (e.g.,
"conditions.reference_time is required", "conditions.rules[%d].name is
required", "conditions.rules[%d].expression is required") and still validate
rule.MaxAge as before so config loading fails fast instead of later during CEL
compilation.
In `@internal/engine/cel_test.go`:
- Around line 350-375: newTestResourceMapWithConditions currently dereferences
conditions[0].Status which panics on an empty slice; modify the function
(newTestResourceMapWithConditions) to guard against empty conditions by checking
len(conditions) > 0 before reading conditions[0].Status and set the
"status.ready" value to false (or appropriate default) when the slice is empty,
while leaving the rest of the payload (including "conditions" -> condList)
intact.
In `@internal/engine/cel.go`:
- Around line 183-209: The compiler must reject empty or non-positive-age rule
sets: before compiling loop verify conditions.Rules is non-empty and return an
error if not; inside the loop validate each rule.MaxAge > 0 and return a clear
error referencing rule.Name/Expression when it's non-positive; initialize minAge
from the first validated rule.MaxAge (or set minAge to a large value then take
min) so minMaxAge in CompiledConditions cannot be zero or negative; ensure the
error messages mention the symbols compiledRule, conditions.Rules, minMaxAge and
the function that builds CompiledConditions so callers (and EvalMaxAge) receive
a deterministic, validated result.
- Around line 173-193: The compile step must enforce expression result types:
when compiling conditions.ReferenceTime (refAst) assert its result type is
string and return a fatal error if not, and when compiling each rule.Expression
(ast) assert its result type is bool and return a fatal error if not; update the
CompileConditions code paths around env.Compile/refAst and env.Compile/ast (and
the associated refPrg/prg creation) to check the AST type information (use the
CEL type-check result on refAst and ast) and include rule.Name and
rule.Expression in the error messages so invalid reference_time or rule
expressions fail at startup instead of causing EvalReferenceTime or EvalMaxAge
to silently misbehave.
In `@internal/engine/decision.go`:
- Around line 25-28: NewDecisionEngine currently stores a nil
*CompiledConditions which later causes a panic when e.conditions is
dereferenced; change NewDecisionEngine to guard against a nil argument by
initializing e.conditions to a safe empty/disabled CompiledConditions (or a
sentinel that always returns a negative decision) when conditions == nil, and
update any methods that dereference e.conditions (the decision-evaluating
methods referenced around lines 57-59 and 85-93) to defensively check
e.conditions != nil and return a safe negative decision instead of panicking;
reference NewDecisionEngine, DecisionEngine, and e.conditions when making the
fixes.
---
Outside diff comments:
In `@internal/config/config_test.go`:
- Around line 277-295: The tests call NewSentinelConfig() and then exercise
duration validation (via Validate()) but never set SentinelConfig.ResourceType,
so Validate() may return "resource_type is required" before negative/zero MaxAge
cases run; update the test setup inside the loop (after cfg :=
NewSentinelConfig() and before tt.modifier(cfg)) to assign a valid
cfg.ResourceType (e.g., a non-empty string) so the duration checks in Validate()
for Rules[0].MaxAge are actually exercised.
In `@test/integration/integration_test.go`:
- Around line 149-170: The tests are short-circuiting on generation mismatch
because DecisionEngine.Evaluate checks r.Generation >
r.Status.ObservedGeneration before evaluating CEL rules; update the fixtures
(createMockClusterWithLabels and the other occurrences mentioned) to set the
top-level resource.Generation and resource.Status.ObservedGeneration
consistently (e.g., set Status.ObservedGeneration = Generation) instead of only
populating per-condition observed_generation so the generation check does not
trigger ReasonGenerationChanged and the new condition_rules logic runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54a79168-3d13-4c1a-9811-abaac1e2d820
📒 Files selected for processing (24)
MakefileREADME.mdcharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/sentinel-operator-guide.mdinternal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/valid-complete.yamlinternal/engine/cel.gointernal/engine/cel_test.gointernal/engine/decision.gointernal/engine/decision_test.gointernal/payload/builder.gointernal/resource/convert.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- configs/rabbitmq-example.yaml
- Makefile
- charts/values.yaml
- internal/sentinel/sentinel.go
4 tasks
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
11b82be to
b2cb37e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/engine/decision.go (1)
24-33:⚠️ Potential issue | 🟠 MajorMake the nil-conditions path actually disable publishing.
The current fallback uses a zero-value
CompiledConditions, soEvalMaxAge()returns0and the engine starts publishing immediately instead of "never publish".e == nilalso still panics before the fallback runs.Proposed fix
func (e *DecisionEngine) Evaluate(r *client.Resource, now time.Time) Decision { + if e == nil || e.conditions == nil { + return Decision{ + ShouldPublish: false, + Reason: "conditions are not configured", + } + } + // Validate inputs if r == nil { return Decision{ ShouldPublish: false, Reason: ReasonNilResource, @@ - // Guard against nil conditions (safe default: never publish) - conditions := e.conditions - if conditions == nil { - conditions = emptyCompiledConditions - } - // Convert resource to map for CEL evaluation resourceMap := resource.ToMap(r) // Evaluate reference time from CEL expression - referenceTime, ok := conditions.EvalReferenceTime(resourceMap) + referenceTime, ok := e.conditions.EvalReferenceTime(resourceMap) @@ - maxAge := conditions.EvalMaxAge(resourceMap) + maxAge := e.conditions.EvalMaxAge(resourceMap)Also applies to: 61-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/decision.go` around lines 24 - 33, NewDecisionEngine currently substitutes a zero-value emptyCompiledConditions which makes EvalMaxAge() return 0 (causing immediate publish) and still allows nil panics elsewhere; change NewDecisionEngine to preserve a nil conditions value when conditions == nil (do not replace with emptyCompiledConditions), remove or set emptyCompiledConditions to nil to avoid misuse, and audit methods that dereference DecisionEngine.conditions (notably EvalMaxAge and any e.conditions.* calls) to treat nil as the "never publish" sentinel (safe-nil checks that return the disabled semantics instead of dereferencing).
🧹 Nitpick comments (3)
charts/README.md (1)
83-84: Call out thatconfig.conditions.rulesis order-sensitive.The current description reads like an unordered schema definition, but this list is behaviorally ordered. Saying “ordered CEL rules; first match wins” would help prevent accidental misconfiguration when rules overlap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/README.md` around lines 83 - 84, Update the README entry for config.conditions.rules to clearly state that rule evaluation is order-sensitive and uses “first match wins” semantics; reference the config.conditions.rules key and values.yaml example so readers know rules are evaluated top-to-bottom and that overlapping rules should be ordered deliberately to avoid accidental matches.internal/config/config_test.go (1)
306-376: Add whitespace-only condition validation cases.These tests cover
"", but the new validation logic also rejects" "viastrings.TrimSpace. I'd add a couple of whitespace-only cases forreference_time,rules[].name, andrules[].expressionso that behavior stays pinned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 306 - 376, Update the tests to include whitespace-only cases because validation now trims input; add new test assertions that provide strings like " " for Conditions.ReferenceTime, for a ConditionRule.Name and for a ConditionRule.Expression and assert the same errors as the empty-string cases (i.e., for ReferenceTime expect "conditions.reference_time is required", for Rules[].Name expect an error containing "name is required", and for Rules[].Expression expect an error containing "expression is required"); modify or add tests near TestValidate_MissingReferenceTime, TestValidate_EmptyRuleName, and TestValidate_EmptyRuleExpression to use strings.TrimSpace-equivalent inputs so the Validate() method is pinned to reject whitespace-only values.test/integration/integration_test.go (1)
149-160: Extract the shared CEL test setup into a helper.The same
config.Conditionsliteral and compile boilerplate is copied into four tests. That makes future rule changes easy to miss and creates two places to keep aligned in each test:cfg.Conditionsand the compiled engine input.♻️ Proposed refactor
+func defaultIntegrationConditions() config.Conditions { + return config.Conditions{ + ReferenceTime: `conditionTime(resource, "Ready")`, + Rules: []config.ConditionRule{ + {Name: "isReady", Expression: `status(resource, "Ready") == "True"`, MaxAge: 30 * time.Minute}, + {Name: "isNotReady", Expression: `status(resource, "Ready") == "False"`, MaxAge: 10 * time.Second}, + }, + } +} + +func mustNewDecisionEngine(t *testing.T, conditions config.Conditions) *engine.DecisionEngine { + t.Helper() + + compiledConditions, err := engine.CompileConditions(conditions) + if err != nil { + t.Fatalf("CompileConditions failed: %v", err) + } + + return engine.NewDecisionEngine(compiledConditions) +}Then each test becomes:
- conditions := config.Conditions{ - ReferenceTime: `conditionTime(resource, "Ready")`, - Rules: []config.ConditionRule{ - {Name: "isReady", Expression: `status(resource, "Ready") == "True"`, MaxAge: 30 * time.Minute}, - {Name: "isNotReady", Expression: `status(resource, "Ready") == "False"`, MaxAge: 10 * time.Second}, - }, - } - compiledConditions, err := engine.CompileConditions(conditions) - if err != nil { - t.Fatalf("CompileConditions failed: %v", err) - } - decisionEngine := engine.NewDecisionEngine(compiledConditions) + conditions := defaultIntegrationConditions() + decisionEngine := mustNewDecisionEngine(t, conditions)Also applies to: 170-170, 275-286, 296-296, 396-407, 416-416, 509-520, 526-526
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/integration_test.go` around lines 149 - 160, Extract the repeated CEL setup into a single test helper (e.g., newTestDecisionEngine or mustCompileConditions) that constructs the config.Conditions literal (with ReferenceTime and Rules), calls engine.CompileConditions and checks err (failing the test on error), then returns the engine.NewDecisionEngine(compiledConditions); replace the inline blocks currently using config.Conditions + engine.CompileConditions + engine.NewDecisionEngine in all tests (those around the shown snippet and the other locations noted) to call this helper instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/resource/convert.go`:
- Around line 11-17: ToMap currently dereferences its input pointer r
immediately (accessing r.Status), which causes a panic when callers (like
BuildPayload) pass nil; update ToMap to check for nil r at the start and return
a safe zero-value map (or a map with nil/zeroed status fields) when r is nil so
callers won't crash; modify the ToMap function to early-return a sensible map if
r == nil and keep existing field formatting (e.g., use empty strings or zero
values for last_transition_time/last_updated when r.Status is absent).
---
Duplicate comments:
In `@internal/engine/decision.go`:
- Around line 24-33: NewDecisionEngine currently substitutes a zero-value
emptyCompiledConditions which makes EvalMaxAge() return 0 (causing immediate
publish) and still allows nil panics elsewhere; change NewDecisionEngine to
preserve a nil conditions value when conditions == nil (do not replace with
emptyCompiledConditions), remove or set emptyCompiledConditions to nil to avoid
misuse, and audit methods that dereference DecisionEngine.conditions (notably
EvalMaxAge and any e.conditions.* calls) to treat nil as the "never publish"
sentinel (safe-nil checks that return the disabled semantics instead of
dereferencing).
---
Nitpick comments:
In `@charts/README.md`:
- Around line 83-84: Update the README entry for config.conditions.rules to
clearly state that rule evaluation is order-sensitive and uses “first match
wins” semantics; reference the config.conditions.rules key and values.yaml
example so readers know rules are evaluated top-to-bottom and that overlapping
rules should be ordered deliberately to avoid accidental matches.
In `@internal/config/config_test.go`:
- Around line 306-376: Update the tests to include whitespace-only cases because
validation now trims input; add new test assertions that provide strings like "
" for Conditions.ReferenceTime, for a ConditionRule.Name and for a
ConditionRule.Expression and assert the same errors as the empty-string cases
(i.e., for ReferenceTime expect "conditions.reference_time is required", for
Rules[].Name expect an error containing "name is required", and for
Rules[].Expression expect an error containing "expression is required"); modify
or add tests near TestValidate_MissingReferenceTime, TestValidate_EmptyRuleName,
and TestValidate_EmptyRuleExpression to use strings.TrimSpace-equivalent inputs
so the Validate() method is pinned to reject whitespace-only values.
In `@test/integration/integration_test.go`:
- Around line 149-160: Extract the repeated CEL setup into a single test helper
(e.g., newTestDecisionEngine or mustCompileConditions) that constructs the
config.Conditions literal (with ReferenceTime and Rules), calls
engine.CompileConditions and checks err (failing the test on error), then
returns the engine.NewDecisionEngine(compiledConditions); replace the inline
blocks currently using config.Conditions + engine.CompileConditions +
engine.NewDecisionEngine in all tests (those around the shown snippet and the
other locations noted) to call this helper instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c36459cc-b6e2-43c4-8f1c-02981d8a68dc
📒 Files selected for processing (24)
MakefileREADME.mdcharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/sentinel-operator-guide.mdinternal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/valid-complete.yamlinternal/engine/cel.gointernal/engine/cel_test.gointernal/engine/decision.gointernal/engine/decision_test.gointernal/payload/builder.gointernal/resource/convert.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- charts/values.yaml
- internal/engine/cel_test.go
- internal/config/testdata/valid-complete.yaml
- internal/config/testdata/full-workflow.yaml
- internal/config/testdata/message-data-blank-id.yaml
- Makefile
- configs/dev-example.yaml
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
b2cb37e to
df1fefa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/engine/cel_test.go (1)
156-182: Add compile-time type-check coverage.
CompileConditions()now rejectsreference_timeexpressions that do not return a string and rule expressions that do not return a bool, but this file only exercises parse failures. A valid CEL AST like1 + 1or"ready"would miss that regression path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/cel_test.go` around lines 156 - 182, Add tests in cel_test.go that exercise CEL type-checking by passing syntactically valid but wrong-typed expressions to CompileConditions: for ReferenceTime use a valid expression that does not return a string (e.g., a numeric literal or arithmetic like `1 + 1`) and assert CompileConditions returns an error; similarly, add a rule with a valid expression that does not return a bool (e.g., a string literal like `"ready"`) and assert CompileConditions returns an error. Target the existing test helpers/TestCompileConditions_InvalidReferenceTime and TestCompileConditions_InvalidRuleExpression or add new tests that call CompileConditions(...) with these cases so the code path that rejects wrong return types is covered.internal/config/config_test.go (1)
306-376: Add whitespace-only cases for the new condition validators.These tests only cover
"", butValidate()now trimsconditions.reference_time,rules[i].name, andrules[i].expression. A" "regression would skip this suite and fail later during CEL compilation instead of at config load time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config_test.go` around lines 306 - 376, Tests for Validate() currently only check empty strings; add whitespace-only cases so trimming logic is exercised: in TestValidate_MissingReferenceTime set cfg.Conditions.ReferenceTime = " " and assert the same "conditions.reference_time is required" error; in TestValidate_EmptyRuleName set the rule Name = " " and assert an error containing "name is required"; and in TestValidate_EmptyRuleExpression set the rule Expression = " " and assert an error containing "expression is required". This ensures Validate() (and its trimming behavior for Conditions.ReferenceTime, ConditionRule.Name, and ConditionRule.Expression) rejects whitespace-only values at config validation time.test/integration/integration_test.go (1)
149-160: Consider extracting duplicated test conditions to a shared helper.The same
config.Conditionsand compilation pattern is repeated identically in all 4 integration tests. Consider extracting to a package-level helper:♻️ Suggested helper function
// Add near the top of the file after imports func createTestConditions(t *testing.T) (*engine.CompiledConditions, config.Conditions) { t.Helper() conditions := config.Conditions{ ReferenceTime: `conditionTime(resource, "Ready")`, Rules: []config.ConditionRule{ {Name: "isReady", Expression: `status(resource, "Ready") == "True"`, MaxAge: 30 * time.Minute}, {Name: "isNotReady", Expression: `status(resource, "Ready") == "False"`, MaxAge: 10 * time.Second}, }, } compiled, err := engine.CompileConditions(conditions) if err != nil { t.Fatalf("CompileConditions failed: %v", err) } return compiled, conditions }Then in each test:
compiledConditions, conditions := createTestConditions(t) decisionEngine := engine.NewDecisionEngine(compiledConditions)Also applies to: 275-286, 396-407, 509-520
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/integration_test.go` around lines 149 - 160, Extract the duplicated config.Conditions setup and compilation into a package-level test helper (e.g., createTestConditions) that takes t *testing.T, calls engine.CompileConditions on the constructed config.Conditions, fails the test on error, and returns the compiled conditions (and optionally the original Conditions). Replace the repeated blocks that build conditions, call engine.CompileConditions, and create a decisionEngine with engine.NewDecisionEngine(compiledConditions) in all four tests to call createTestConditions(t) and use its returned compiled conditions to construct the DecisionEngine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/engine/decision.go`:
- Around line 32-33: The sentinel emptyCompiledConditions currently uses
zero-valued minMaxAge causing EvalMaxAge to return 0 and incorrectly allow
publishing; initialize emptyCompiledConditions so its
CompiledConditions.minMaxAge is set to a very large duration (e.g., a Duration
built from math.MaxInt64 nanoseconds) to implement true "never publish"
semantics, adjust imports (math/time) as needed, and leave other
CompiledConditions fields unchanged so EvalMaxAge and subsequent
nextEventTime/ShouldPublish logic behave as intended.
In `@internal/resource/convert.go`:
- Around line 21-55: The code currently omits status["conditions"] when
r.Status.Conditions is empty; change the logic so that conditions is always set
(an empty []interface{} when there are no conditions) instead of only assigning
when len(r.Status.Conditions) > 0. Specifically, build conditions :=
make([]interface{}, len(r.Status.Conditions)) and populate it in the loop, but
remove the conditional gate and always set status["conditions"] = conditions;
also ensure any initial nil/default status map uses an empty slice for
"conditions" instead of omitting the key. This preserves the resource shape for
status and prevents CEL missing-field errors when accessing
resource.status.conditions.
In `@Makefile`:
- Around line 219-223: Update the Helm smoke test to set the new values under
the config.conditions path instead of the removed config.conditionRules path:
replace the --set flag that uses config.conditionRules.rules[0].maxAge=1h with
the equivalent new key (e.g., --set config.conditions[0].maxAge=1h) so the helm
template command (the line with "helm template test-release ...") actually
overrides the custom condition rule and validates the chart.
---
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 306-376: Tests for Validate() currently only check empty strings;
add whitespace-only cases so trimming logic is exercised: in
TestValidate_MissingReferenceTime set cfg.Conditions.ReferenceTime = " " and
assert the same "conditions.reference_time is required" error; in
TestValidate_EmptyRuleName set the rule Name = " " and assert an error
containing "name is required"; and in TestValidate_EmptyRuleExpression set the
rule Expression = " " and assert an error containing "expression is required".
This ensures Validate() (and its trimming behavior for Conditions.ReferenceTime,
ConditionRule.Name, and ConditionRule.Expression) rejects whitespace-only values
at config validation time.
In `@internal/engine/cel_test.go`:
- Around line 156-182: Add tests in cel_test.go that exercise CEL type-checking
by passing syntactically valid but wrong-typed expressions to CompileConditions:
for ReferenceTime use a valid expression that does not return a string (e.g., a
numeric literal or arithmetic like `1 + 1`) and assert CompileConditions returns
an error; similarly, add a rule with a valid expression that does not return a
bool (e.g., a string literal like `"ready"`) and assert CompileConditions
returns an error. Target the existing test
helpers/TestCompileConditions_InvalidReferenceTime and
TestCompileConditions_InvalidRuleExpression or add new tests that call
CompileConditions(...) with these cases so the code path that rejects wrong
return types is covered.
In `@test/integration/integration_test.go`:
- Around line 149-160: Extract the duplicated config.Conditions setup and
compilation into a package-level test helper (e.g., createTestConditions) that
takes t *testing.T, calls engine.CompileConditions on the constructed
config.Conditions, fails the test on error, and returns the compiled conditions
(and optionally the original Conditions). Replace the repeated blocks that build
conditions, call engine.CompileConditions, and create a decisionEngine with
engine.NewDecisionEngine(compiledConditions) in all four tests to call
createTestConditions(t) and use its returned compiled conditions to construct
the DecisionEngine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1d8234d-e1e2-4576-af1d-5fb2bd8f620c
📒 Files selected for processing (24)
MakefileREADME.mdcharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/sentinel-operator-guide.mdinternal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/valid-complete.yamlinternal/engine/cel.gointernal/engine/cel_test.gointernal/engine/decision.gointernal/engine/decision_test.gointernal/payload/builder.gointernal/resource/convert.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/config/testdata/full-workflow.yaml
- charts/README.md
- charts/templates/configmap.yaml
- configs/dev-example.yaml
- internal/sentinel/sentinel.go
- README.md
Comment on lines
+32
to
+33
| // emptyCompiledConditions is a safe sentinel for nil conditions | ||
| var emptyCompiledConditions = &CompiledConditions{} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify EvalMaxAge behavior with empty CompiledConditions
ast-grep --pattern $'func ($_ *CompiledConditions) EvalMaxAge($_) $_ {
$$$
}'Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 899
🏁 Script executed:
#!/bin/bash
# Check if there's special handling for empty rules in EvalMaxAge
rg -n -A 10 'func.*EvalMaxAge' --type goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 3533
🏁 Script executed:
#!/bin/bash
# Get context from decision.go around the mentioned lines
cat -n internal/engine/decision.go | head -110 | tail -85Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 3687
🏁 Script executed:
#!/bin/bash
# Check how ShouldPublish uses the minMaxAge
rg -n -B 5 -A 15 'func.*ShouldPublish' internal/engine/decision.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
#!/bin/bash
# See the rest of the Evaluate function
sed -n '109,130p' internal/engine/decision.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 377
🏁 Script executed:
#!/bin/bash
# Find CompiledConditions struct definition to see default minMaxAge value
rg -n -A 10 'type CompiledConditions struct' --type goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 653
🏁 Script executed:
#!/bin/bash
# Check if minMaxAge is ever set to a safe default value elsewhere
rg -n 'minMaxAge' --type goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Check how minMaxAge is set in CompileConditions
sed -n '190,230p' internal/engine/cel.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1642
🏁 Script executed:
#!/bin/bash
# Look for the full CompileConditions function to understand minAge calculation
ast-grep --pattern $'func CompileConditions($_) ($_, $_) {
$$$
}'Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 3991
🏁 Script executed:
#!/bin/bash
# Check if there's a way emptyCompiledConditions is supposed to be used differently
# or if there are tests that cover nil conditions behavior
rg -n 'emptyCompiledConditions' --type goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Check tests to see if nil conditions (never publish) behavior is tested
rg -n -B 5 -A 15 'never.*publish\|nil.*condition' --type go -i internal/engine/decision_test.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Look for any tests related to decision engine and nil conditions
rg -n -B 3 -A 10 'TestEvaluate\|emptyCompiledConditions\|nil.*[Cc]ondition' internal/engine/decision_test.go | head -50Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Check if there's any comment or documentation about the "never publish" intent
rg -n -B 2 -A 2 'safe.*default\|never.*publish' internal/engine/decision.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
#!/bin/bash
# Check if decision_test.go exists and what tests cover the Evaluate function
fd 'decision_test.go' && cat -n internal/engine/decision_test.go | head -150Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 5345
🏁 Script executed:
#!/bin/bash
# Get the exact comment text around line 86
sed -n '80,92p' internal/engine/decision.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 382
emptyCompiledConditions does not implement "never publish" semantics.
The comment at line 86 states the guard uses emptyCompiledConditions as a "safe default: never publish" when conditions are nil. However, with minMaxAge zero-valued to 0, EvalMaxAge returns 0, causing nextEventTime = referenceTime.Add(0) (line 106). When referenceTime falls back to r.CreatedTime (line 99), the condition now.Before(nextEventTime) at line 109 evaluates to false for any resource created in the past, resulting in ShouldPublish: true at lines 119–122, contradicting the documented intent.
To enforce true "never publish" semantics, emptyCompiledConditions.minMaxAge must be set to a very large duration (e.g., math.MaxInt64 nanoseconds) so that nextEventTime lies far in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/engine/decision.go` around lines 32 - 33, The sentinel
emptyCompiledConditions currently uses zero-valued minMaxAge causing EvalMaxAge
to return 0 and incorrectly allow publishing; initialize emptyCompiledConditions
so its CompiledConditions.minMaxAge is set to a very large duration (e.g., a
Duration built from math.MaxInt64 nanoseconds) to implement true "never publish"
semantics, adjust imports (math/time) as needed, and leave other
CompiledConditions fields unchanged so EvalMaxAge and subsequent
nextEventTime/ShouldPublish logic behave as intended.
Comment on lines
+21
to
+55
| "status": map[string]interface{}{ | ||
| "ready": false, | ||
| "last_transition_time": "", | ||
| "last_updated": "", | ||
| "observed_generation": int32(0), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| status := map[string]interface{}{ | ||
| "ready": r.Status.Ready, | ||
| "last_transition_time": r.Status.LastTransitionTime.Format(time.RFC3339Nano), | ||
| "last_updated": r.Status.LastUpdated.Format(time.RFC3339Nano), | ||
| "observed_generation": r.Status.ObservedGeneration, | ||
| } | ||
| if len(r.Status.Conditions) > 0 { | ||
| conditions := make([]interface{}, len(r.Status.Conditions)) | ||
| for i, c := range r.Status.Conditions { | ||
| cond := map[string]interface{}{ | ||
| "type": c.Type, | ||
| "status": c.Status, | ||
| "last_transition_time": c.LastTransitionTime.Format(time.RFC3339Nano), | ||
| "last_updated_time": c.LastUpdatedTime.Format(time.RFC3339Nano), | ||
| "observed_generation": c.ObservedGeneration, | ||
| } | ||
| if c.Reason != "" { | ||
| cond["reason"] = c.Reason | ||
| } | ||
| if c.Message != "" { | ||
| cond["message"] = c.Message | ||
| } | ||
| conditions[i] = cond | ||
| } | ||
| status["conditions"] = conditions | ||
| } |
There was a problem hiding this comment.
Always expose status.conditions, even when empty.
Line 36 currently drops the field when a resource has no conditions, and the nil fallback does the same. That turns a valid “no conditions yet” state into a missing-field CEL error, so expressions that traverse resource.status.conditions can fail instead of evaluating against []. Initializing it to an empty list keeps the resource shape stable for both payload and decision evaluation.
Suggested fix
if r == nil {
return map[string]interface{}{
"id": "",
"href": "",
"kind": "",
"created_time": "",
"updated_time": "",
"generation": int32(0),
"status": map[string]interface{}{
"ready": false,
"last_transition_time": "",
"last_updated": "",
"observed_generation": int32(0),
+ "conditions": []interface{}{},
},
}
}
status := map[string]interface{}{
"ready": r.Status.Ready,
"last_transition_time": r.Status.LastTransitionTime.Format(time.RFC3339Nano),
"last_updated": r.Status.LastUpdated.Format(time.RFC3339Nano),
"observed_generation": r.Status.ObservedGeneration,
+ "conditions": []interface{}{},
}
if len(r.Status.Conditions) > 0 {
conditions := make([]interface{}, len(r.Status.Conditions))
for i, c := range r.Status.Conditions {
cond := map[string]interface{}{
"type": c.Type,
"status": c.Status,
"last_transition_time": c.LastTransitionTime.Format(time.RFC3339Nano),
"last_updated_time": c.LastUpdatedTime.Format(time.RFC3339Nano),
"observed_generation": c.ObservedGeneration,
}
if c.Reason != "" {
cond["reason"] = c.Reason
}
if c.Message != "" {
cond["message"] = c.Message
}
conditions[i] = cond
}
status["conditions"] = conditions
}📝 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
| "status": map[string]interface{}{ | |
| "ready": false, | |
| "last_transition_time": "", | |
| "last_updated": "", | |
| "observed_generation": int32(0), | |
| }, | |
| } | |
| } | |
| status := map[string]interface{}{ | |
| "ready": r.Status.Ready, | |
| "last_transition_time": r.Status.LastTransitionTime.Format(time.RFC3339Nano), | |
| "last_updated": r.Status.LastUpdated.Format(time.RFC3339Nano), | |
| "observed_generation": r.Status.ObservedGeneration, | |
| } | |
| if len(r.Status.Conditions) > 0 { | |
| conditions := make([]interface{}, len(r.Status.Conditions)) | |
| for i, c := range r.Status.Conditions { | |
| cond := map[string]interface{}{ | |
| "type": c.Type, | |
| "status": c.Status, | |
| "last_transition_time": c.LastTransitionTime.Format(time.RFC3339Nano), | |
| "last_updated_time": c.LastUpdatedTime.Format(time.RFC3339Nano), | |
| "observed_generation": c.ObservedGeneration, | |
| } | |
| if c.Reason != "" { | |
| cond["reason"] = c.Reason | |
| } | |
| if c.Message != "" { | |
| cond["message"] = c.Message | |
| } | |
| conditions[i] = cond | |
| } | |
| status["conditions"] = conditions | |
| } | |
| "status": map[string]interface{}{ | |
| "ready": false, | |
| "last_transition_time": "", | |
| "last_updated": "", | |
| "observed_generation": int32(0), | |
| "conditions": []interface{}{}, | |
| }, | |
| } | |
| } | |
| status := map[string]interface{}{ | |
| "ready": r.Status.Ready, | |
| "last_transition_time": r.Status.LastTransitionTime.Format(time.RFC3339Nano), | |
| "last_updated": r.Status.LastUpdated.Format(time.RFC3339Nano), | |
| "observed_generation": r.Status.ObservedGeneration, | |
| "conditions": []interface{}{}, | |
| } | |
| if len(r.Status.Conditions) > 0 { | |
| conditions := make([]interface{}, len(r.Status.Conditions)) | |
| for i, c := range r.Status.Conditions { | |
| cond := map[string]interface{}{ | |
| "type": c.Type, | |
| "status": c.Status, | |
| "last_transition_time": c.LastTransitionTime.Format(time.RFC3339Nano), | |
| "last_updated_time": c.LastUpdatedTime.Format(time.RFC3339Nano), | |
| "observed_generation": c.ObservedGeneration, | |
| } | |
| if c.Reason != "" { | |
| cond["reason"] = c.Reason | |
| } | |
| if c.Message != "" { | |
| cond["message"] = c.Message | |
| } | |
| conditions[i] = cond | |
| } | |
| status["conditions"] = conditions | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/resource/convert.go` around lines 21 - 55, The code currently omits
status["conditions"] when r.Status.Conditions is empty; change the logic so that
conditions is always set (an empty []interface{} when there are no conditions)
instead of only assigning when len(r.Status.Conditions) > 0. Specifically, build
conditions := make([]interface{}, len(r.Status.Conditions)) and populate it in
the loop, but remove the conditional gate and always set status["conditions"] =
conditions; also ensure any initial nil/default status map uses an empty slice
for "conditions" instead of omitting the key. This preserves the resource shape
for status and prevents CEL missing-field errors when accessing
resource.status.conditions.
Replace the fixed condition_type/status/max_age structure with CEL-based conditions using reference_time and expression rules, aligning with the adapter's preconditions pattern. - Extract resourceToMap() to internal/resource/convert.go (shared) - Add CEL environment with condition(), status(), conditionTime() custom functions (internal/engine/cel.go) - Replace config.ConditionRules with config.Conditions (reference_time + rules with name/expression/max_age) - Update DecisionEngine to use CompiledConditions with CEL eval - Compile CEL expressions at startup (fail-fast) - Update configs, Helm chart, and docs
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
df1fefa to
cce3ed1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/engine/decision.go (1)
24-29:⚠️ Potential issue | 🟠 MajorReturn a negative decision before any publish path when conditions are missing.
Line 79can still publish on a generation mismatch before the nil-conditions fallback runs, and the zero-value sentinel onLine 33makes the time-based path publish immediately for older resources as well. That still contradicts the "never publish" contract documented inNewDecisionEngine.Suggested fix
-// emptyCompiledConditions is a safe sentinel for nil conditions -var emptyCompiledConditions = &CompiledConditions{} - // Decision represents the result of evaluating a resource type Decision struct { @@ func (e *DecisionEngine) Evaluate(r *client.Resource, now time.Time) Decision { // Validate inputs if r == nil { @@ if now.IsZero() { return Decision{ ShouldPublish: false, Reason: ReasonZeroNow, } } + + if e == nil || e.conditions == nil { + return Decision{ + ShouldPublish: false, + Reason: "conditions are not configured", + } + } // Check for generation mismatch // This triggers immediate reconciliation regardless of max age if r.Generation > r.Status.ObservedGeneration { @@ - // Guard against nil conditions (safe default: never publish) conditions := e.conditions - if conditions == nil { - conditions = emptyCompiledConditions - } // Convert resource to map for CEL evaluation resourceMap := resource.ToMap(r)Also applies to: 32-33, 61-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/decision.go` around lines 24 - 29, NewDecisionEngine promises "never publish" when conditions is nil but the decision logic still allows publishing via generation-mismatch or time-based checks; update the DecisionEngine's decision method (the routine that inspects generation and timestamp) to perform an early guard: if de.conditions == nil then immediately return a negative/no-publish decision (short-circuit) before any generation-mismatch or age-based logic runs; also ensure any zero-value time/generation sentinels are treated as "no publish" when conditions is nil so the default does not cause immediate publishes.
🧹 Nitpick comments (1)
internal/engine/cel.go (1)
253-272: Consider logging rule evaluation errors for observability.
EvalMaxAgesilently continues when a rule evaluation fails (line 264). While this is appropriate for runtime resilience, consider adding debug-level logging to aid troubleshooting when rules fail to evaluate.💡 Optional: Add observability for rule evaluation failures
If you have access to a logger in this context, consider:
for _, rule := range cc.rules { out, _, err := rule.program.Eval(activation) if err != nil { + // Consider adding: log.V(2).Infof("Rule %s evaluation failed: %v", rule.name, err) continue }This would help operators understand why rules aren't matching when debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/engine/cel.go` around lines 253 - 272, EvalMaxAge currently swallows errors from rule.program.Eval (in CompiledConditions.EvalMaxAge) which hinders observability; modify the error handling in the loop over cc.rules to emit a debug-level log when rule.program.Eval returns err (include the error and a rule identifier such as rule.name or rule.maxAge and any rule index), and ensure logging is conditional on a logger being available (use an injected/logger field on CompiledConditions or a package logger if one exists); keep the existing continue behavior after logging so runtime resilience is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/engine/decision.go`:
- Around line 24-29: NewDecisionEngine promises "never publish" when conditions
is nil but the decision logic still allows publishing via generation-mismatch or
time-based checks; update the DecisionEngine's decision method (the routine that
inspects generation and timestamp) to perform an early guard: if de.conditions
== nil then immediately return a negative/no-publish decision (short-circuit)
before any generation-mismatch or age-based logic runs; also ensure any
zero-value time/generation sentinels are treated as "no publish" when conditions
is nil so the default does not cause immediate publishes.
---
Nitpick comments:
In `@internal/engine/cel.go`:
- Around line 253-272: EvalMaxAge currently swallows errors from
rule.program.Eval (in CompiledConditions.EvalMaxAge) which hinders
observability; modify the error handling in the loop over cc.rules to emit a
debug-level log when rule.program.Eval returns err (include the error and a rule
identifier such as rule.name or rule.maxAge and any rule index), and ensure
logging is conditional on a logger being available (use an injected/logger field
on CompiledConditions or a package logger if one exists); keep the existing
continue behavior after logging so runtime resilience is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8afb139f-db18-44d5-8145-a839c9e85079
📒 Files selected for processing (24)
MakefileREADME.mdcharts/README.mdcharts/templates/configmap.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamldocs/sentinel-operator-guide.mdinternal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/message-data-blank-id.yamlinternal/config/testdata/valid-complete.yamlinternal/engine/cel.gointernal/engine/cel_test.gointernal/engine/decision.gointernal/engine/decision_test.gointernal/payload/builder.gointernal/resource/convert.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- README.md
- configs/gcp-pubsub-example.yaml
- charts/README.md
- configs/dev-example.yaml
- internal/config/testdata/message-data-blank-id.yaml
- internal/config/testdata/valid-complete.yaml
- charts/templates/configmap.yaml
rh-amarin
reviewed
Mar 12, 2026
| // | ||
| // Adapter Contract: | ||
| // - Adapters MUST update status.LastUpdated on EVERY evaluation | ||
| // - Adapters MUST update condition LastUpdatedTime on EVERY evaluation |
Contributor
There was a problem hiding this comment.
Why do we have here info on the adapter's behaviour?
IMO there is risk of this drifting from changes in adapter
rh-amarin
reviewed
Mar 12, 2026
| # Max age interval for resources that are ready and stable. | ||
| # Stable resources are checked less frequently to reduce API load. | ||
| max_age_ready: 30m | ||
| # Condition rules for decision making. |
Contributor
There was a problem hiding this comment.
I haven't found how are condition combined, is it AND or OR?
rh-amarin
reviewed
Mar 12, 2026
| # status(resource, type) → string - returns condition status | ||
| # conditionTime(resource, type) → string - returns last_updated_time (RFC3339) | ||
| conditions: | ||
| reference_time: 'conditionTime(resource, "Ready")' |
Contributor
There was a problem hiding this comment.
Is not clear to me what reference_time is used for, I guess is to compare with the max_age property?
- But in this case, these are really a mix between CEL expressions and some hardcoded rules
- How does the
expressionandmax_agecombine in the same rule?
In my mind, the CEL engine could be more generic.
- Instead of the hardcoded
reference_timethere could be aparamssection to define custom variables to use later in other rules - `conditionTime(resource, "Ready") does here too much magic
- Traverses the resource object and decides between 2 properties
One idea, but I think we should brainstorm this:
rules:
params:
- name: max_age
expression: 30m
- name: ready
expression: resource.conditions.filter(c, c.type=="Ready")[0]
- name: ref_time
expression: has(ready.last_updated_time )
? ready.last_updated_time
: ready.created_time
conditions:
- operator: AND
- name: state-strategy
expression: ready != "True"
- name: time-based-strategy
expression: ref_time > max_age
- operator: OR
- name: xxx
expression: expr1
- name: yy
expression: expr2
rules:
params:
- name: max_age
expression: 30m
- name: ready
expression: resource.conditions.filter(c, c.type=="Ready")[0]
- name: ref_time
expression: has(ready.last_updated_time )
? ready.last_updated_time
: ready.created_time
- name: operand1
expression: ready != "True"
conditions:
- name: state-strategy
expression: operand1 AND operand2 AND (operand3 OR operand4)
trigger_decission:
- name: max_age
expression: 30m
- name: ready
expression: resource.conditions.filter(c, c.type=="Ready")[0]
- name: ref_time
expression: has(ready.last_updated_time )
? ready.last_updated_time
: ready.created_time
- name: operand1
expression: ready != "True"
- name: result
expression: operand1 AND operand2 AND (operand3 OR operand4)
- name: reason
expression: xxxx
Contributor
Author
There was a problem hiding this comment.
My proposal:
trigger_decission:
- name: max_age
expression: 30m
- name: ready
expression: resource.conditions.filter(c, c.type=="Ready")[0]
- name: ref_time
expression: has(ready.last_updated_time )
? ready.last_updated_time
: ready.created_time
- name: operand1
expression: ready != "True"
result:
expression: operand1 AND operand2 AND (operand3 OR operand4)
reason:
expression: xxxxOR
message_decision:
params:
max_age: 30m
ready: resource.conditions.filter(c, c.type=="Ready")[0]
ref_time: has(ready.last_updated_time )
? ready.last_updated_time
: ready.created_time
operand1: ready != "True"
result: operand1 AND operand2 AND (operand3 OR operand4)
Contributor
Author
|
This PR Will continue after https://issues.redhat.com/browse/HYPERFLEET-536 |
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.
Summary
condition_rules(fixedcondition_type/status/max_age) intoconditionswith CEL expressions (reference_time+ rules withname/expression/max_age), aligning with the adapter's preconditions patternresourceToMap()to sharedinternal/resource/convert.go(used by both payload builder and decision engine)condition(),status(),conditionTime()(internal/engine/cel.go)Config change
Before:
After:
Test plan
go test ./internal/... ./cmd/...)make lint— 0 issues)status == "True" && status == "True")Summary by CodeRabbit
New Features
Documentation
Configuration
Charts