HYPERFLEET-643 - feat: allow payload nested structure#49
HYPERFLEET-643 - feat: allow payload nested structure#49rh-amarin wants to merge 2 commits 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 Go-template message_data with a CEL-based payload builder across configs and testdata (message mappings become CEL expressions like "resource.id"). Adds computed fields ( Sequence Diagram(s)sequenceDiagram
autonumber
participant Sentinel as Sentinel
participant Client as API Client
participant Builder as PayloadBuilder
participant CEL as CEL Env
participant Broker as Broker/Publisher
Sentinel->>Client: Fetch resource
Client-->>Sentinel: Resource (with OwnerReferences)
alt MessageData configured
Sentinel->>Builder: BuildPayload(resource, reason)
Builder->>CEL: Evaluate compiled expressions against resource+reason
CEL-->>Builder: Evaluation results (map)
Builder-->>Sentinel: Built payload (map)
else MessageData nil
Sentinel-->>Sentinel: Use minimal hardcoded payload
end
Sentinel->>Broker: Publish CloudEvent(payload)
Broker-->>Sentinel: Publish result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
configs/rabbitmq-example.yaml (1)
47-61:⚠️ Potential issue | 🟡 MinorUpdate the message_data comment to reflect CEL (not Go templates).
The comment block still mentions Go template syntax, which no longer applies.
📝 Suggested doc fix
-# Message data template configuration (optional). -# Define custom fields to include in CloudEvents data payload. -# Uses Go template syntax - both .field and {{.field}} formats are supported. -# Templates can reference any field from the Resource object returned by the API. +# Message data configuration (optional). +# Define custom fields to include in CloudEvents data payload. +# Each value is a CEL expression evaluated against "resource" and "reason". +# A nested map builds a nested object — each leaf value is still a CEL expression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/rabbitmq-example.yaml` around lines 47 - 61, Update the comment above the message_data block to state that templates use CEL expressions (not Go templates); mention that fields reference the Resource object and that both dot notation like resource.field and CEL expressions (e.g., resource.status.ready ? "Ready" : "NotReady") are supported—identify the message_data section and examples such as ready_status and origin so the comment clearly reflects CEL semantics rather than Go template syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/dev-example.yaml`:
- Around line 64-66: The config uses the wrong key name "owned_reference" so it
won't match the Resource JSON field; rename the key to "owner_references" and
adjust the structure to match the expected Resource schema (i.e., use the
owner_references array/object shape and include the id and kind inside each
owner entry), updating any references in code or templates that read
"owned_reference" to "owner_references" so the runtime will resolve the values
correctly.
In `@internal/client/client.go`:
- Around line 428-441: The code dereferences item.OwnerReferences.Id/.Href/.Kind
without checking item.OwnerReferences for nil, which can panic; add a nil guard
around the whole extraction (e.g., if item.OwnerReferences != nil) before
reading its fields, then perform the existing nil checks on
item.OwnerReferences.Id, .Href, .Kind to populate the local OwnerReference
struct and assign it to resource.OwnerReferences only when non-empty.
In `@internal/config/testdata/full-workflow.yaml`:
- Around line 16-20: The cluster_id path is wrong: message_data currently
references resource.owned_reference.id which is a typo and will evaluate to
null; update the field to use the actual owner reference array
(resource.owner_references) and pick the correct identifier (for example
resource.owner_references[0].uid or .id depending on the schema used in the
builder tests) so cluster_id pulls from the real owner reference value instead
of a non-existent property.
In `@internal/payload/builder.go`:
- Around line 135-165: The nested-object case in evalCompiledNode currently
returns an empty map when all child leaves are omitted, causing callers to
include {} instead of omitting the key; change evalCompiledNode to call
evalCompiledMap(node.children, ...) and if that returned map has length 0 return
nil (so the parent evalCompiledMap will skip it) otherwise return the map;
reference compiledNode.children, evalCompiledMap and evalCompiledNode when
making this change.
---
Outside diff comments:
In `@configs/rabbitmq-example.yaml`:
- Around line 47-61: Update the comment above the message_data block to state
that templates use CEL expressions (not Go templates); mention that fields
reference the Resource object and that both dot notation like resource.field and
CEL expressions (e.g., resource.status.ready ? "Ready" : "NotReady") are
supported—identify the message_data section and examples such as ready_status
and origin so the comment clearly reflects CEL semantics rather than Go template
syntax.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
configs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlgo.modinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/valid-complete.yamlinternal/payload/builder.gointernal/payload/builder_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.go
| // Extract owner references (present on NodePool, references parent Cluster) | ||
| ownerRef := &OwnerReference{} | ||
| if item.OwnerReferences.Id != nil { | ||
| ownerRef.ID = *item.OwnerReferences.Id | ||
| } | ||
| if item.OwnerReferences.Href != nil { | ||
| ownerRef.Href = *item.OwnerReferences.Href | ||
| } | ||
| if item.OwnerReferences.Kind != nil { | ||
| ownerRef.Kind = *item.OwnerReferences.Kind | ||
| } | ||
| if ownerRef.ID != "" || ownerRef.Href != "" || ownerRef.Kind != "" { | ||
| resource.OwnerReferences = ownerRef | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and inspect the OpenAPI model for OwnerReferences
rg -n "OwnerReferences" pkg/api/openapi -C 3Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 139
🏁 Script executed:
# Also check for snake_case variant
rg -n "owner_references" pkg/api/openapi -C 3Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 139
🏁 Script executed:
# Examine the full context of the code in internal/client/client.go around lines 428-441
cat -n internal/client/client.go | sed -n '410,460p'Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1868
🏁 Script executed:
# Find the function containing line 428 and the type of item
sed -n '370,430p' internal/client/client.go | head -n 60Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1852
🏁 Script executed:
# Search for OwnerReferences type definition in the entire codebase
rg -n "type.*OwnerReferences" --type go -C 3Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
# Also search for struct definitions with OwnerReferences field
rg -n "OwnerReferences" --type go | head -20Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1475
🏁 Script executed:
# Find the function signature to confirm item type
sed -n '350,430p' internal/client/client.go | grep -n "func\|for\|:=.*item"Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 338
🏁 Script executed:
# Look for the item type declaration more broadly
sed -n '300,450p' internal/client/client.go | head -100Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 2728
🏁 Script executed:
# Search for OpenAPI type definitions - look for where item type is defined
fd -e go | xargs rg -l "openapi\." | head -5Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 109
🏁 Script executed:
# Find the openapi model file for NodePool
rg -l "type.*NodePool" --type goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
# Look for the OpenAPI generated type definition
rg -n "type.*struct" --type go | grep -i "nodep\|ownerref"Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 142
🏁 Script executed:
# Find the OwnerReference struct definition
rg -n "type OwnerReference struct" -A 5 internal/client/client.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 218
🏁 Script executed:
# Search for NodePool type definition - likely in openapi generated code or imports
rg -n "NodePool" internal/client/client.go | head -10Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 710
🏁 Script executed:
# Find what openapi package is being used
rg -n "^import" -A 20 internal/client/client.go | head -30Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 639
🏁 Script executed:
# Search for NodePool type in the openapi package
find . -path "*/pkg/api/openapi*" -name "*.go" -type fRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
# Look for NodePool struct definition and OwnerReferences field
rg -n "type.*NodePool" --type go -C 3Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
# Inspect what OwnerReferences looks like in the openapi model
rg -n "OwnerReferences" --type go -B 2 -A 2 | grep -A 5 -B 5 "openapi"Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 66
🏁 Script executed:
# List files in pkg/api directory to find openapi models
find pkg -type f -name "*.go" 2>/dev/null | head -20Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 196
🏁 Script executed:
# Check if openapi is a vendored or embedded package
find . -name "openapi*" -type d 2>/dev/nullRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 93
🏁 Script executed:
# Search for where item is declared in fetchNodePools - get full function signature
sed -n '323,345p' internal/client/client.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 758
🏁 Script executed:
# Search for NodePool type in the openapi directory
find ./openapi -name "*.go" -type f | xargs rg -l "NodePool"Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 260
🏁 Script executed:
# Find OwnerReferences field definition in openapi types
find ./openapi -name "*.go" -type f | xargs rg -n "OwnerReferences" -B 2 -A 2Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 2988
🏁 Script executed:
# Get the full NodePool struct definition
find ./openapi -name "*.go" -type f | xargs rg -n "type.*NodePool.*struct" -A 30Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 68
🏁 Script executed:
# Check test data to see if OwnerReferences is provided in API responses
sed -n '550,650p' internal/client/client_test.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 3585
🏁 Script executed:
# Look at what test data is used for NodePool responses
rg -n "OwnerReferences" internal/client/client_test.go -B 10 -A 10 | head -50Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 1293
Add nil guard before accessing item.OwnerReferences fields.
The code at lines 430–437 accesses item.OwnerReferences.Id, .Href, and .Kind without checking if item.OwnerReferences itself is nil. Since OwnerReferences is a pointer type (line 76) with omitempty in the JSON tag, it can be nil in API responses. This will cause a panic if the field is absent.
The surrounding code already guards other optional pointer fields (item.Id, item.Href, item.Kind, item.Labels) with nil checks—OwnerReferences should follow the same pattern:
if item.OwnerReferences != nil {
if item.OwnerReferences.Id != nil {
ownerRef.ID = *item.OwnerReferences.Id
}
if item.OwnerReferences.Href != nil {
ownerRef.Href = *item.OwnerReferences.Href
}
if item.OwnerReferences.Kind != nil {
ownerRef.Kind = *item.OwnerReferences.Kind
}
if ownerRef.ID != "" || ownerRef.Href != "" || ownerRef.Kind != "" {
resource.OwnerReferences = ownerRef
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/client.go` around lines 428 - 441, The code dereferences
item.OwnerReferences.Id/.Href/.Kind without checking item.OwnerReferences for
nil, which can panic; add a nil guard around the whole extraction (e.g., if
item.OwnerReferences != nil) before reading its fields, then perform the
existing nil checks on item.OwnerReferences.Id, .Href, .Kind to populate the
local OwnerReference struct and assign it to resource.OwnerReferences only when
non-empty.
There was a problem hiding this comment.
Code will not compile with error: invalid operation: mismatched types openapi.ObjectReference and untyped nil
Tested and it also doesn't panic
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the type of OwnerReferences in the OpenAPI model
rg -n "type.*ObjectReference.*struct" --type go -A 10
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for OwnerReferences field definition in openapi generated files
find . -path "*/pkg/api/openapi*" -name "*.go" -exec grep -n "OwnerReferences" {} + | head -20
# Also check the specific type used in the NodePool item
rg -n "OwnerReferences.*\`json" --type go -B 2 -A 2
Length of output: 402
🏁 Script executed:
#!/bin/bash
# Find all Go files that might contain OpenAPI models
find . -name "*.go" -path "*/openapi*" -o -name "*.go" -path "*/pkg/api/*" | head -20
# Search for NodePool struct definition and its OwnerReferences field
rg -n "type NodePool" --type go -A 30 | grep -A 30 "struct"
# Look for ObjectReference type definition
rg -n "ObjectReference" --type go -C 3 | head -50
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Let's check the imports and see what openapi package is being used
head -30 internal/client/client.go | grep -A 20 "import"
# Also examine how item is declared in the fetchNodePools function
sed -n '350,370p' internal/client/client.go
Length of output: 1150
🏁 Script executed:
#!/bin/bash
# Let's verify the exact type by looking at how OwnerReferences behaves in the test
rg -n "OwnerReferences" internal/client/client_test.go -B 5 -A 5 | head -60
Length of output: 846
🏁 Script executed:
#!/bin/bash
# Check the openapi package structure to understand the ObjectReference type
ls -la pkg/api/openapi/ 2>/dev/null || find . -path "*/pkg/api/openapi*" -type f -name "*.go" | head -5
# Try to find where ObjectReference is defined or used
rg -n "ObjectReference" --type go | head -20
Length of output: 0
@rh-amarin, you're absolutely right—I apologize for the incorrect suggestion.
The item.OwnerReferences field is a struct value type (openapi.ObjectReference), not a pointer, so it cannot be nil. Comparing it to nil would indeed fail to compile with the exact error you mentioned. The existing code is correct: it checks whether the individual pointer fields (.Id, .Href, .Kind) inside the struct are nil, which is the proper pattern for this scenario.
Thank you for testing and catching this!
rh-amarin
force-pushed
the
HYPERFLEET-353
branch
from
45c3a50 to
443c070
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/client/client.go (1)
428-441:⚠️ Potential issue | 🔴 CriticalAdd nil guard before accessing
item.OwnerReferencesfields.
item.OwnerReferencesmay be nil when the API response omits the field (pointer type withomitempty). Dereferencing.Id,.Href,.Kindon a nil pointer will panic. Wrap the entire block in anif item.OwnerReferences != nilguard, consistent with how other optional pointer fields (item.Id,item.Href,item.Kind) are handled above.Proposed fix
// Extract owner references (present on NodePool, references parent Cluster) - ownerRef := &OwnerReference{} - if item.OwnerReferences.Id != nil { - ownerRef.ID = *item.OwnerReferences.Id - } - if item.OwnerReferences.Href != nil { - ownerRef.Href = *item.OwnerReferences.Href - } - if item.OwnerReferences.Kind != nil { - ownerRef.Kind = *item.OwnerReferences.Kind - } - if ownerRef.ID != "" || ownerRef.Href != "" || ownerRef.Kind != "" { - resource.OwnerReferences = ownerRef + if item.OwnerReferences != nil { + ownerRef := &OwnerReference{} + if item.OwnerReferences.Id != nil { + ownerRef.ID = *item.OwnerReferences.Id + } + if item.OwnerReferences.Href != nil { + ownerRef.Href = *item.OwnerReferences.Href + } + if item.OwnerReferences.Kind != nil { + ownerRef.Kind = *item.OwnerReferences.Kind + } + if ownerRef.ID != "" || ownerRef.Href != "" || ownerRef.Kind != "" { + resource.OwnerReferences = ownerRef + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client.go` around lines 428 - 441, The block that reads fields from item.OwnerReferences can panic if item.OwnerReferences is nil; wrap the existing extraction logic in an if item.OwnerReferences != nil { ... } guard so you only dereference item.OwnerReferences.Id/Href/Kind when non-nil, populate the local OwnerReference and then set resource.OwnerReferences = ownerRef as before; this mirrors how item.Id/item.Href/item.Kind are safely handled and prevents runtime nil-pointer dereference in the OwnerReference handling code.internal/payload/builder.go (1)
149-152:⚠️ Potential issue | 🟡 MinorOmit empty nested objects to stay consistent with "silently omitted" contract.
When all children of a nested node evaluate to
nil(e.g., all referenced fields are missing),evalCompiledMapreturns an emptymap[string]interface{}{}. Since an empty map is notnil, the parentevalCompiledMapwill include it as"key": {}rather than omitting it. This contradicts the documented behavior that missing field paths are silently omitted.Proposed fix
func evalCompiledNode(node *compiledNode, resourceMap map[string]interface{}, reason string) interface{} { if node.children != nil { - return evalCompiledMap(node.children, resourceMap, reason) + child := evalCompiledMap(node.children, resourceMap, reason) + if len(child) == 0 { + return nil + } + return child }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/payload/builder.go` around lines 149 - 152, In evalCompiledNode (and/or the caller evalCompiledMap), change the handling of nested objects so that when node.children is non-nil you call evalCompiledMap(node.children, resourceMap, reason) and if that call returns a map[string]interface{} with zero entries treat it as nil (return nil) instead of the empty map; this ensures empty nested objects produced by evalCompiledMap are omitted per the "silently omitted" contract. Refer to compiledNode, evalCompiledNode and evalCompiledMap when making the change.internal/sentinel/sentinel_test.go (1)
401-401:metrics.NewSentinelMetrics(registry)may need aversionparameter after rebase.A past review noted that PR
#50changed the signature tometrics.NewSentinelMetrics(registry, version). This applies to all call sites in this file (lines 138, 205, 249, 298, 346, 401, 453, 516). After rebasing onto main, these will fail to compile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel_test.go` at line 401, Update every call to metrics.NewSentinelMetrics in sentinel_test.go to match the new signature by passing the version argument as the second parameter (metrics.NewSentinelMetrics(registry, version)); locate calls to NewSentinelMetrics and supply an appropriate version value (reuse an existing test/service version variable or define a local test version constant) so the calls compile with the updated function signature.
🧹 Nitpick comments (3)
go.mod (1)
19-23: Direct dependencycel-gois placed in an indirectrequireblock.
github.com/google/cel-gois a direct dependency (no// indirectcomment on line 60), but it's listed inside the thirdrequireblock alongside actual indirect dependencies rather than the first block (lines 5–17) where other direct dependencies live. This won't break anything—go mod tidycontrols the// indirectmarker—but grouping it with the other direct dependencies improves readability.Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 19 - 23, The go.mod places the direct dependency github.com/google/cel-go inside a secondary require block with indirect deps; move the require line for github.com/google/cel-go from the third require group into the primary require block where other direct dependencies are declared (ensure it remains without the // indirect comment), so the direct dependency is grouped with other direct requires for readability.internal/payload/builder.go (1)
122-133: JSON round-trip coerces numeric types tofloat64.
json.Unmarshalintomap[string]interface{}converts all JSON numbers tofloat64. Soresource.generation(originallyint32) will befloat64in CEL evaluation context, andtime.Timefields become strings. This is generally fine for CEL's dynamic typing but worth a brief doc comment so future maintainers aren't surprised when comparing types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/payload/builder.go` around lines 122 - 133, The JSON round-trip in BuildPayload (json.Marshal + json.Unmarshal into map[string]interface{}) coerces all numbers to float64 and converts time.Time to strings, so fields like resource.generation (int32) become float64 and time.Time fields become strings in the CEL evaluation context; update the comment above BuildPayload (and mention evalCompiledMap and b.compiled) to document this coercion and its implications for CEL comparisons so future maintainers aren’t surprised, and note an alternative (e.g., use json.Decoder.UseNumber or a custom marshal-to-dyn step) if exact numeric types are required.internal/config/config_test.go (1)
64-77: Consider centralizing repeatedMessageDatamap assertions in a test helper.The cast/fail boilerplate is duplicated and can be reduced for maintainability.
♻️ Suggested refactor
+func requireMessageDataMap(t *testing.T, v interface{}) map[string]interface{} { + t.Helper() + md, ok := v.(map[string]interface{}) + if !ok { + t.Fatalf("Expected message_data to be map[string]interface{}, got %T", v) + } + return md +} + func TestLoadConfig_ValidComplete(t *testing.T) { @@ - md, ok := cfg.MessageData.(map[string]interface{}) - if !ok { - t.Fatalf("Expected message_data to be map[string]interface{}, got %T", cfg.MessageData) - } + md := requireMessageDataMap(t, cfg.MessageData) @@ func TestLoadConfig_FullWorkflow(t *testing.T) { @@ - md, ok := cfg.MessageData.(map[string]interface{}) - if !ok { - t.Fatalf("Expected message_data to be map[string]interface{}, got %T", cfg.MessageData) - } + md := requireMessageDataMap(t, cfg.MessageData)Also applies to: 420-426
🤖 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 64 - 77, Extract the repeated cast-and-assert logic around cfg.MessageData into a small test helper (e.g., getMessageDataMap or assertMessageDataMap) that accepts testing.TB and the cfg, performs the type assertion on cfg.MessageData to map[string]interface{} (failing the test with t.Fatalf on mismatch), and returns the map for further assertions; then replace the duplicated blocks in internal/config/config_test.go (including the current block referencing cfg.MessageData and the other occurrence around lines 420-426) with calls to that helper so downstream checks (length, resource_id value, etc.) operate on the returned map.
🤖 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/client/client.go`:
- Around line 428-441: The block that reads fields from item.OwnerReferences can
panic if item.OwnerReferences is nil; wrap the existing extraction logic in an
if item.OwnerReferences != nil { ... } guard so you only dereference
item.OwnerReferences.Id/Href/Kind when non-nil, populate the local
OwnerReference and then set resource.OwnerReferences = ownerRef as before; this
mirrors how item.Id/item.Href/item.Kind are safely handled and prevents runtime
nil-pointer dereference in the OwnerReference handling code.
In `@internal/payload/builder.go`:
- Around line 149-152: In evalCompiledNode (and/or the caller evalCompiledMap),
change the handling of nested objects so that when node.children is non-nil you
call evalCompiledMap(node.children, resourceMap, reason) and if that call
returns a map[string]interface{} with zero entries treat it as nil (return nil)
instead of the empty map; this ensures empty nested objects produced by
evalCompiledMap are omitted per the "silently omitted" contract. Refer to
compiledNode, evalCompiledNode and evalCompiledMap when making the change.
In `@internal/sentinel/sentinel_test.go`:
- Line 401: Update every call to metrics.NewSentinelMetrics in sentinel_test.go
to match the new signature by passing the version argument as the second
parameter (metrics.NewSentinelMetrics(registry, version)); locate calls to
NewSentinelMetrics and supply an appropriate version value (reuse an existing
test/service version variable or define a local test version constant) so the
calls compile with the updated function signature.
---
Nitpick comments:
In `@go.mod`:
- Around line 19-23: The go.mod places the direct dependency
github.com/google/cel-go inside a secondary require block with indirect deps;
move the require line for github.com/google/cel-go from the third require group
into the primary require block where other direct dependencies are declared
(ensure it remains without the // indirect comment), so the direct dependency is
grouped with other direct requires for readability.
In `@internal/config/config_test.go`:
- Around line 64-77: Extract the repeated cast-and-assert logic around
cfg.MessageData into a small test helper (e.g., getMessageDataMap or
assertMessageDataMap) that accepts testing.TB and the cfg, performs the type
assertion on cfg.MessageData to map[string]interface{} (failing the test with
t.Fatalf on mismatch), and returns the map for further assertions; then replace
the duplicated blocks in internal/config/config_test.go (including the current
block referencing cfg.MessageData and the other occurrence around lines 420-426)
with calls to that helper so downstream checks (length, resource_id value, etc.)
operate on the returned map.
In `@internal/payload/builder.go`:
- Around line 122-133: The JSON round-trip in BuildPayload (json.Marshal +
json.Unmarshal into map[string]interface{}) coerces all numbers to float64 and
converts time.Time to strings, so fields like resource.generation (int32) become
float64 and time.Time fields become strings in the CEL evaluation context;
update the comment above BuildPayload (and mention evalCompiledMap and
b.compiled) to document this coercion and its implications for CEL comparisons
so future maintainers aren’t surprised, and note an alternative (e.g., use
json.Decoder.UseNumber or a custom marshal-to-dyn step) if exact numeric types
are required.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
configs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlgo.modinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/valid-complete.yamlinternal/payload/builder.gointernal/payload/builder_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/client/client_test.go
- configs/rabbitmq-example.yaml
- internal/config/testdata/full-workflow.yaml
- internal/config/testdata/valid-complete.yaml
- internal/sentinel/sentinel.go
rh-amarin
force-pushed
the
HYPERFLEET-353
branch
from
443c070 to
c5a85fd
Compare
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)
internal/sentinel/sentinel_test.go (1)
592-646:⚠️ Potential issue | 🟡 Minor
TestTrigger_ContextPropagationToBrokerdoes not exerciseSentinel.trigger— test name is misleading.The test constructs no
Sentineland never callstrigger(). It directly invokesmockPublisherWithLogger.Publish(ctx, ...), which trivially re-passesctxto the mock logger. The assertion that context values survive is tautologically true — the samectxobject is passed in and immediately captured.The actual propagation path (
trigger→ context enrichment →publisher.Publish) is left uncovered. Consider either:
- Wiring a real
SentinelwithMockPublisherWithLoggerand triggering it, or- Renaming the test to accurately reflect what it validates (e.g.,
TestMockPublisherWithLogger_ContextPassthrough).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel_test.go` around lines 592 - 646, The test TestTrigger_ContextPropagationToBroker currently calls MockPublisherWithLogger.Publish directly instead of exercising Sentinel.trigger; update the test to construct a real Sentinel instance (using MockPublisherWithLogger as its publisher) and invoke s.trigger(ctx, event, ...) so the code path through Sentinel.trigger → context enrichment → publisher.Publish is exercised, then assert on capturedContexts; alternatively, if you prefer not to call trigger, rename the test to reflect it's only validating MockPublisherWithLogger.Publish passthrough (e.g., TestMockPublisherWithLogger_ContextPassthrough).
♻️ Duplicate comments (3)
internal/client/client.go (1)
428-441:⚠️ Potential issue | 🔴 CriticalGuard
item.OwnerReferencesbefore dereferencing nested fields.Line 430 accesses
item.OwnerReferences.Idwithout checking whetheritem.OwnerReferencesis nil, which can panic on NodePools that do not include owner references.🛠️ Suggested fix
// Extract owner references (present on NodePool, references parent Cluster) ownerRef := &OwnerReference{} - if item.OwnerReferences.Id != nil { - ownerRef.ID = *item.OwnerReferences.Id - } - if item.OwnerReferences.Href != nil { - ownerRef.Href = *item.OwnerReferences.Href - } - if item.OwnerReferences.Kind != nil { - ownerRef.Kind = *item.OwnerReferences.Kind + if item.OwnerReferences != nil { + if item.OwnerReferences.Id != nil { + ownerRef.ID = *item.OwnerReferences.Id + } + if item.OwnerReferences.Href != nil { + ownerRef.Href = *item.OwnerReferences.Href + } + if item.OwnerReferences.Kind != nil { + ownerRef.Kind = *item.OwnerReferences.Kind + } } if ownerRef.ID != "" || ownerRef.Href != "" || ownerRef.Kind != "" { resource.OwnerReferences = ownerRef }#!/bin/bash # Verify current dereference pattern and optionality of OwnerReferences in generated models. rg -n "item\\.OwnerReferences\\." internal/client/client.go -C3 rg -n "OwnerReferences" pkg/api/openapi -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/client/client.go` around lines 428 - 441, The code dereferences item.OwnerReferences (e.g., item.OwnerReferences.Id/Href/Kind) without checking for nil, which can panic; update the block that builds ownerRef (and sets resource.OwnerReferences) to first guard with a nil check on item.OwnerReferences, e.g., only access Id/Href/Kind when item.OwnerReferences != nil, then populate OwnerReference fields and assign resource.OwnerReferences; reference the OwnerReference struct, item.OwnerReferences, and resource.OwnerReferences when locating the change.internal/payload/builder.go (1)
150-152:⚠️ Potential issue | 🟡 MinorReturn
nilfor empty nested maps to preserve omission semantics.Line 151 currently returns
{}for nested nodes whose children all resolve to omitted values, so the parent key is still emitted as an empty object.🛠️ Suggested fix
func evalCompiledNode(node *compiledNode, resourceMap map[string]interface{}, reason string) interface{} { if node.children != nil { - return evalCompiledMap(node.children, resourceMap, reason) + child := evalCompiledMap(node.children, resourceMap, reason) + if len(child) == 0 { + return nil + } + return child }#!/bin/bash # Verify nested-map evaluation path and omission docs. sed -n '147,156p' internal/payload/builder.go rg -n "silently omitted|omitted" internal/payload/builder.go configs/dev-example.yaml configs/gcp-pubsub-example.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/payload/builder.go` around lines 150 - 152, The nested-map branch currently returns whatever evalCompiledMap(node.children, resourceMap, reason) produces even when it is an empty map, causing parent keys to be emitted as {} instead of being omitted; change the node.children handling in the function to call evalCompiledMap into a local variable (e.g., childVal), check if childVal is a map with len==0 (or otherwise represents an omitted value) and return nil in that case, otherwise return childVal so empty nested maps are treated as omitted; reference symbols: node.children and evalCompiledMap(node.children, resourceMap, reason).internal/sentinel/sentinel_test.go (1)
401-401:⚠️ Potential issue | 🔴 Critical
metrics.NewSentinelMetricsmissingversionargument — compile failure after rebase.All three new test functions call
metrics.NewSentinelMetrics(registry)with a single argument. Per PR#50(HYPERFLEET-427), the signature was changed toNewSentinelMetrics(registry, version). These will fail to compile once this branch is rebased onto the updatedmain.🛠️ Proposed fix
- metrics.NewSentinelMetrics(registry) + metrics.NewSentinelMetrics(registry, "test")(Apply to lines 401, 453, and 516.)
Also applies to: 453-453, 516-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel_test.go` at line 401, The test calls to metrics.NewSentinelMetrics are missing the new required second parameter; update each call (e.g., in the tests that invoke metrics.NewSentinelMetrics(registry)) to pass the version argument (for example metrics.NewSentinelMetrics(registry, version) or metrics.NewSentinelMetrics(registry, "test") / a testVersion const), ensuring the signature matches the updated function; modify all occurrences that call NewSentinelMetrics (including the three failing tests) to include the version value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/sentinel/sentinel_test.go`:
- Around line 592-646: The test TestTrigger_ContextPropagationToBroker currently
calls MockPublisherWithLogger.Publish directly instead of exercising
Sentinel.trigger; update the test to construct a real Sentinel instance (using
MockPublisherWithLogger as its publisher) and invoke s.trigger(ctx, event, ...)
so the code path through Sentinel.trigger → context enrichment →
publisher.Publish is exercised, then assert on capturedContexts; alternatively,
if you prefer not to call trigger, rename the test to reflect it's only
validating MockPublisherWithLogger.Publish passthrough (e.g.,
TestMockPublisherWithLogger_ContextPassthrough).
---
Duplicate comments:
In `@internal/client/client.go`:
- Around line 428-441: The code dereferences item.OwnerReferences (e.g.,
item.OwnerReferences.Id/Href/Kind) without checking for nil, which can panic;
update the block that builds ownerRef (and sets resource.OwnerReferences) to
first guard with a nil check on item.OwnerReferences, e.g., only access
Id/Href/Kind when item.OwnerReferences != nil, then populate OwnerReference
fields and assign resource.OwnerReferences; reference the OwnerReference struct,
item.OwnerReferences, and resource.OwnerReferences when locating the change.
In `@internal/payload/builder.go`:
- Around line 150-152: The nested-map branch currently returns whatever
evalCompiledMap(node.children, resourceMap, reason) produces even when it is an
empty map, causing parent keys to be emitted as {} instead of being omitted;
change the node.children handling in the function to call evalCompiledMap into a
local variable (e.g., childVal), check if childVal is a map with len==0 (or
otherwise represents an omitted value) and return nil in that case, otherwise
return childVal so empty nested maps are treated as omitted; reference symbols:
node.children and evalCompiledMap(node.children, resourceMap, reason).
In `@internal/sentinel/sentinel_test.go`:
- Line 401: The test calls to metrics.NewSentinelMetrics are missing the new
required second parameter; update each call (e.g., in the tests that invoke
metrics.NewSentinelMetrics(registry)) to pass the version argument (for example
metrics.NewSentinelMetrics(registry, version) or
metrics.NewSentinelMetrics(registry, "test") / a testVersion const), ensuring
the signature matches the updated function; modify all occurrences that call
NewSentinelMetrics (including the three failing tests) to include the version
value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
configs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlgo.modinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/valid-complete.yamlinternal/payload/builder.gointernal/payload/builder_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/client/client_test.go
- internal/payload/builder_test.go
- go.mod
- configs/rabbitmq-example.yaml
- internal/config/testdata/valid-complete.yaml
| resource_type: .kind | ||
| href: .href | ||
| generation: .generation | ||
| id: "resource.id" |
There was a problem hiding this comment.
Maybe jsonPath can work better than Cel here?
There was a problem hiding this comment.
For example in this file, later we have
ready_status: 'resource.status.ready == true ? "Ready" : "NotReady"'
IMO CEL expressions are more powerful, and I'm concerned about offering too many options to use may be more confusing than helpful. wdyt?
| @@ -65,15 +65,16 @@ func NewHyperFleetClient(endpoint string, timeout time.Duration) (*HyperFleetCli | |||
|
|
|||
| // Resource represents a HyperFleet resource (cluster, nodepool, etc.) | |||
| type Resource struct { | |||
There was a problem hiding this comment.
If we use jsonPath do we still need the client structs? We don't need to update our sentinel no matter how openAPI changes, and only configurations need to be changed.
There was a problem hiding this comment.
By jsonPath do you mean parsing the HyperFleet API responses as JSON structures?
I think that may come if we make the API "flexible" to accept generic objects and not only cluster/nodepools
Then, we will change the API and the Sentinel to be more similar to adapters which work with JSON
So, I would consider that to be a different ticket than this one
| log.Warn(ctx, "message_data is empty, CloudEvents will have minimal data payload") | ||
| // ValidateMessageData validates the message_data build definition at startup. | ||
| // Returns nil if message_data is not configured (fallback mode). | ||
| func (c *SentinelConfig) ValidateMessageData() error { |
There was a problem hiding this comment.
Can we make this part configurable? like
message_data:
id:
field: "resource.id"
type: string
must_have: true
kind:
field: "resource.kind"
type: string
must_have: true
default: "cluster"
...
There was a problem hiding this comment.
IMO if we want to validate the message format, we may use the CloudEvent spec
https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/components/broker/asyncapi.yaml
There was a problem hiding this comment.
It can be a topic if we would like to keep that. Or we can make cloudEvents messagedata fully managed by service offering team.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sentinel/sentinel_test.go (1)
490-493: Strengthen negative assertions for fallback fields.This currently checks only
reasonis absent. It would be safer to also assertid,kind,generation, andhrefare absent when builder config is active.Suggested test hardening
// Hardcoded fields should NOT be present when builder is configured - if _, ok := data["reason"]; ok { - t.Errorf("Expected 'reason' to be absent (hardcoded field), but found it") - } + for _, field := range []string{"id", "kind", "generation", "href", "reason"} { + if _, ok := data[field]; ok { + t.Errorf("Expected %q to be absent (hardcoded fallback field), but found it", field) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel_test.go` around lines 490 - 493, Extend the negative assertion that currently checks only data["reason"] to also verify the keys "id", "kind", "generation", and "href" are not present when the builder config is active: add checks similar to the existing if _, ok := data["reason"]; ok { ... } pattern for data["id"], data["kind"], data["generation"], and data["href"] in the same test (where the builder configuration is applied) so the test fails if any of those hardcoded/fallback fields appear.
🤖 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_test.go`:
- Around line 490-493: Extend the negative assertion that currently checks only
data["reason"] to also verify the keys "id", "kind", "generation", and "href"
are not present when the builder config is active: add checks similar to the
existing if _, ok := data["reason"]; ok { ... } pattern for data["id"],
data["kind"], data["generation"], and data["href"] in the same test (where the
builder configuration is applied) so the test fails if any of those
hardcoded/fallback fields appear.
| @@ -49,11 +49,16 @@ hyperfleet_api: | |||
| # Uses Go template syntax - both .field and {{.field}} formats are supported. | |||
| # Templates can reference any field from the Resource object returned by the API. | |||
| message_data: | |||
There was a problem hiding this comment.
The message_data values in this file were migrated to CEL syntax, but the
comment block above still references Go template syntax:
# Uses Go template syntax - both .field and {{.field}} formats are supported.
# Templates can reference any field from the Resource object returned by the API.This should be updated to match the CEL documentation used in dev-example.yaml and gcp-pubsub-example.yaml.
There was a problem hiding this comment.
Good catch!
I also updated the README where go templates were mentioned and added the variables in scope for the CEL engine
| ResourceSelector LabelSelectorList `mapstructure:"resource_selector"` | ||
| HyperFleetAPI *HyperFleetAPIConfig `mapstructure:"hyperfleet_api"` | ||
| MessageData map[string]string `mapstructure:"message_data"` | ||
| MessageData interface{} `mapstructure:"message_data"` |
There was a problem hiding this comment.
Consider using map[string]interface{} instead of interface{} for the
MessageData field type. Viper's mapstructure already decodes nested YAML maps
as map[string]interface{}, so there's no practical reason to use the broader
interface{} type. This would:
- Avoid the need for type assertions in tests and in NewBuilder
- Catch invalid top-level types (like a bare string) at the config
unmarshalling stage rather than at validation time - Make the nil check for "not configured" still work the same way
| MessageData interface{} `mapstructure:"message_data"` | |
| MessageData map[string]interface{} `mapstructure:"message_data"` |
| // BuildPayload builds an event payload map from the given resource and decision reason. | ||
| // The reason is available to CEL expressions as the "reason" variable. | ||
| func (b *Builder) BuildPayload(resource *client.Resource, reason string) (map[string]interface{}, error) { | ||
| // JSON round-trip to get a plain map[string]interface{} from the resource struct. |
There was a problem hiding this comment.
BuildPayload does a json.Marshal → json.Unmarshal round-trip on every resource
to convert the struct into a map[string]interface{} for CEL evaluation. This
works correctly but generates two temporary []byte allocations plus full
reflection per resource per polling cycle.
A few alternatives to consider (not necessarily for this PR, but worth a
follow-up):
- Build the map manually from Resource fields (avoids reflection overhead
entirely) - Use a library like mapstructure to convert struct → map without JSON
serialization - Declare typed CEL variables instead of DynType so CEL can work with the
struct directly
Not blocking, but worth keeping in mind as the number of monitored resources grows.
| # kind: "resource.owner_references.kind" | ||
| # href: "resource.owner_references.href" | ||
| ready_status: 'resource.status.ready == true ? "Ready" : "NotReady"' | ||
| origin: '"hyperfleet-sentinel"' |
| # id: "resource.owner_references.id" | ||
| # kind: "resource.owner_references.kind" | ||
| # href: "resource.owner_references.href" | ||
| ready_status: 'resource.status.ready == true ? "Ready" : "NotReady"' |
There was a problem hiding this comment.
Is this example only meant to demonstrate a CEL expression?
For the clusters/nodepools messageData, does it actually exclude ready_status and origin? Are these two properties added purely for illustration purposes?
rh-amarin
force-pushed
the
HYPERFLEET-353
branch
from
39a3ede to
5b910a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
243-248:⚠️ Potential issue | 🟡 MinorUpdate example to use CEL syntax.
The "Production Configuration with Sharding" example still uses the old Go template syntax (
.id,.kind, etc.) which is inconsistent with the CEL migration documented earlier in the same section.📝 Suggested fix
message_data: - resource_id: .id - resource_type: .kind - href: .href - generation: .generation + resource_id: "resource.id" + resource_type: "resource.kind" + href: "resource.href" + generation: "resource.generation"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 243 - 248, The example uses old Go template placeholders (.id, .kind, .href, .generation); update it to CEL expressions instead. Replace the keys under message_data to use CEL accessors (e.g., resource.id, resource.kind, resource.href, resource.generation) so resource_id -> resource.id, resource_type -> resource.kind, href -> resource.href, and generation -> resource.generation, keeping the same key names (message_data, resource_id, resource_type, href, generation).
♻️ Duplicate comments (1)
configs/rabbitmq-example.yaml (1)
49-50:⚠️ Potential issue | 🟡 MinorComment still contains misleading syntax reference.
Line 49 states "both .field and {{.field}} formats are supported" which was Go template syntax. For CEL, expressions use
resource.fieldformat. The comment should match the CEL documentation ingcp-pubsub-example.yamlwhich correctly explains CEL variables and evaluation.📝 Suggested fix
-# Uses CEL syntax - both .field and {{.field}} formats are supported. -# Templates can reference any field from the Resource object returned by the API. +# Each property value is a CEL expression evaluated against "resource" and "reason". +# A nested map builds a nested object — each leaf value is still a CEL expression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/rabbitmq-example.yaml` around lines 49 - 50, Update the misleading comment in configs/rabbitmq-example.yaml that currently says "both .field and {{.field}} formats are supported" to correctly describe CEL syntax (use resource.field expressions) so it matches the CEL documentation and the comment in gcp-pubsub-example.yaml; locate the comment block around the template documentation and replace the Go-template references with a short explanation that CEL expressions reference fields as resource.field and can be used directly in templates.
🧹 Nitpick comments (4)
internal/payload/builder.go (1)
25-38: Consider handling unsigned integer types.
ParseValueDefhandlesint,int32,int64,float32,float64,bool, but doesn't handle unsigned integers (uint,uint32,uint64). Depending on the YAML parser, some numeric values might be parsed as unsigned types.♻️ Suggested enhancement
func ParseValueDef(raw interface{}) (*ValueDef, error) { switch v := raw.(type) { case string: return &ValueDef{Expression: v}, nil - case int, int32, int64, float32, float64, bool: + case int, int32, int64, uint, uint32, uint64, float32, float64, bool: return &ValueDef{Literal: v}, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/payload/builder.go` around lines 25 - 38, ParseValueDef currently omits unsigned integer types, so update the type switch in ParseValueDef to include uint, uint32, uint64 (and optionally uintptr) alongside existing numeric cases so unsigned numeric values are treated as Literal; ensure you reference the ValueDef{Literal: v} branch and keep the existing nil/map/string handling unchanged.internal/payload/builder_test.go (1)
280-309: Add a nilowner_referencescase to protect optional-field behavior.
owner_referencesis optional, but current coverage only validates the populated path. A dedicated nil-case test will prevent regressions where payload generation errors or emits unexpected keys.➕ Suggested test addition
+func TestBuildPayload_NodePool_NoOwnerReferences(t *testing.T) { + buildDef := map[string]interface{}{ + "id": "resource.id", + "cluster_id": "resource.owner_references.id", + "cluster_kind": "resource.owner_references.kind", + } + b, err := NewBuilder(buildDef) + if err != nil { + t.Fatalf("NewBuilder failed: %v", err) + } + + resource := makeTestNodePoolResource() + resource.OwnerReferences = nil + + payload, err := b.BuildPayload(resource, "") + if err != nil { + t.Fatalf("BuildPayload failed: %v", err) + } + + if _, exists := payload["cluster_id"]; exists { + t.Errorf("expected cluster_id to be omitted when owner_references is nil") + } + if _, exists := payload["cluster_kind"]; exists { + t.Errorf("expected cluster_kind to be omitted when owner_references is nil") + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/payload/builder_test.go` around lines 280 - 309, Add a new test similar to TestBuildPayload_NodePool_OwnerReferences that uses NewBuilder and b.BuildPayload but passes a node pool resource whose owner_references field is nil (use makeTestNodePoolResource as a base and set owner_references to nil/omit it); assert BuildPayload returns no error and that payload does not contain cluster_id/cluster_kind (or they are empty/zero values) and that id/kind still produce expected values, preventing regressions when owner_references is optional.internal/sentinel/sentinel.go (1)
45-53: Avoid silent fallback whenmessage_datawas explicitly configured.At Line 49, downgrading to hardcoded payload after builder init failure can unexpectedly change the event contract. Prefer failing sentinel initialization when custom payload setup fails.
🔧 Suggested direction
-func NewSentinel(...) *Sentinel { +func NewSentinel(...) (*Sentinel, error) { ... if cfg.MessageData != nil { builder, err := payload.NewBuilder(cfg.MessageData) if err != nil { - ctx := context.Background() - log.Warnf(ctx, "Failed to create payload builder: %v, using hardcoded payload", err) + return nil, fmt.Errorf("failed to create payload builder: %w", err) } s.payloadBuilder = builder } - return s + return s, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sentinel/sentinel.go` around lines 45 - 53, The current sentinel initialization silently falls back to a hardcoded payload when payload.NewBuilder(cfg.MessageData) fails; instead, modify the sentinel constructor/initializer (the function that sets s.payloadBuilder) to return an error if payload.NewBuilder returns an error so initialization fails fast. Replace the log-and-fallback behavior with propagating the error (include context about cfg.MessageData and the builder failure), set s.payloadBuilder only on success, and update callers of the constructor/initializer to handle the returned error accordingly.internal/config/config_test.go (1)
420-426: Strengthen full-workflowmessage_dataassertions beyondlen(md).Only checking size can hide key/shape regressions. Add assertions for expected keys and nested map types so this test catches schema drift.
🤖 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 420 - 426, The test currently only checks len(md) for cfg.MessageData; instead, assert that cfg.MessageData (cast to md) contains the exact expected keys (e.g., "key1","key2","key3","key4") and validate each key's value type and nested structure by checking types for nested maps/slices (for example assert md["someNested"] is map[string]interface{} and its expected sub-keys exist), and replace the loose len(md) assertion with explicit existence and type checks so the test will fail on missing keys or shape regressions; reference cfg.MessageData and the md variable when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 151-154: The Markdown list for CEL variables is misformatted: add
a space after the dash so items render as proper list entries; update the README
line referencing the CEL variables (the `resource` and `reason` entries) to use
"- `resource` : ..." and "- `reason` : ..." so both items follow correct
Markdown list syntax and render properly in the CEL variable list.
---
Outside diff comments:
In `@README.md`:
- Around line 243-248: The example uses old Go template placeholders (.id,
.kind, .href, .generation); update it to CEL expressions instead. Replace the
keys under message_data to use CEL accessors (e.g., resource.id, resource.kind,
resource.href, resource.generation) so resource_id -> resource.id, resource_type
-> resource.kind, href -> resource.href, and generation -> resource.generation,
keeping the same key names (message_data, resource_id, resource_type, href,
generation).
---
Duplicate comments:
In `@configs/rabbitmq-example.yaml`:
- Around line 49-50: Update the misleading comment in
configs/rabbitmq-example.yaml that currently says "both .field and {{.field}}
formats are supported" to correctly describe CEL syntax (use resource.field
expressions) so it matches the CEL documentation and the comment in
gcp-pubsub-example.yaml; locate the comment block around the template
documentation and replace the Go-template references with a short explanation
that CEL expressions reference fields as resource.field and can be used directly
in templates.
---
Nitpick comments:
In `@internal/config/config_test.go`:
- Around line 420-426: The test currently only checks len(md) for
cfg.MessageData; instead, assert that cfg.MessageData (cast to md) contains the
exact expected keys (e.g., "key1","key2","key3","key4") and validate each key's
value type and nested structure by checking types for nested maps/slices (for
example assert md["someNested"] is map[string]interface{} and its expected
sub-keys exist), and replace the loose len(md) assertion with explicit existence
and type checks so the test will fail on missing keys or shape regressions;
reference cfg.MessageData and the md variable when adding these checks.
In `@internal/payload/builder_test.go`:
- Around line 280-309: Add a new test similar to
TestBuildPayload_NodePool_OwnerReferences that uses NewBuilder and
b.BuildPayload but passes a node pool resource whose owner_references field is
nil (use makeTestNodePoolResource as a base and set owner_references to nil/omit
it); assert BuildPayload returns no error and that payload does not contain
cluster_id/cluster_kind (or they are empty/zero values) and that id/kind still
produce expected values, preventing regressions when owner_references is
optional.
In `@internal/payload/builder.go`:
- Around line 25-38: ParseValueDef currently omits unsigned integer types, so
update the type switch in ParseValueDef to include uint, uint32, uint64 (and
optionally uintptr) alongside existing numeric cases so unsigned numeric values
are treated as Literal; ensure you reference the ValueDef{Literal: v} branch and
keep the existing nil/map/string handling unchanged.
In `@internal/sentinel/sentinel.go`:
- Around line 45-53: The current sentinel initialization silently falls back to
a hardcoded payload when payload.NewBuilder(cfg.MessageData) fails; instead,
modify the sentinel constructor/initializer (the function that sets
s.payloadBuilder) to return an error if payload.NewBuilder returns an error so
initialization fails fast. Replace the log-and-fallback behavior with
propagating the error (include context about cfg.MessageData and the builder
failure), set s.payloadBuilder only on success, and update callers of the
constructor/initializer to handle the returned error accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
README.mdconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlgo.modinternal/client/client.gointernal/client/client_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/testdata/full-workflow.yamlinternal/config/testdata/valid-complete.yamlinternal/payload/builder.gointernal/payload/builder_test.gointernal/sentinel/sentinel.gointernal/sentinel/sentinel_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- configs/dev-example.yaml
- internal/client/client_test.go
- internal/sentinel/sentinel_test.go
README.md
Outdated
| Define custom fields to include in CloudEvents using CEL syntax. The evaluation engine has access to variables: | ||
| -`resource` : contains the resource fetched from HyperFleet API | ||
|
|
||
| - `reason` : decisions outcome of Sentinel for the resource |
There was a problem hiding this comment.
Fix formatting in CEL variable list.
Line 152 has a formatting issue: -resource should be `- `resource (space after dash for proper Markdown list formatting).
📝 Suggested fix
Define custom fields to include in CloudEvents using CEL syntax. The evaluation engine has access to variables:
--`resource` : contains the resource fetched from HyperFleet API
+- `resource` : contains the resource fetched from HyperFleet API
- `reason` : decisions outcome of Sentinel for the resource📝 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.
| Define custom fields to include in CloudEvents using CEL syntax. The evaluation engine has access to variables: | |
| -`resource` : contains the resource fetched from HyperFleet API | |
| - `reason` : decisions outcome of Sentinel for the resource | |
| Define custom fields to include in CloudEvents using CEL syntax. The evaluation engine has access to variables: | |
| - `resource` : contains the resource fetched from HyperFleet API | |
| - `reason` : decisions outcome of Sentinel for the resource |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 151 - 154, The Markdown list for CEL variables is
misformatted: add a space after the dash so items render as proper list entries;
update the README line referencing the CEL variables (the `resource` and
`reason` entries) to use "- `resource` : ..." and "- `reason` : ..." so both
items follow correct Markdown list syntax and render properly in the CEL
variable list.
internal/sentinel/sentinel.go
Outdated
| builder, err := payload.NewBuilder(cfg.MessageData) | ||
| if err != nil { | ||
| ctx := context.Background() | ||
| log.Warnf(ctx, "Failed to create payload builder: %v, using hardcoded payload", err) |
There was a problem hiding this comment.
Is there a reason to only warn here and not return?
There was a problem hiding this comment.
Should NewSentinel return an error 🤔
| } | ||
|
|
||
| // evalCompiledMap evaluates a compiled map against the resource and reason. | ||
| func evalCompiledMap(nodes map[string]*compiledNode, resourceMap map[string]interface{}, reason string) map[string]interface{} { |
There was a problem hiding this comment.
Could we get a test for this func 🙏
I am also wondering should this func omit owner_references if no parent exists, instead of returning owner_references : {}
internal/config/config.go
Outdated
| if err := cfg.ValidateTemplates(); err != nil { | ||
| return nil, fmt.Errorf("invalid message_data templates: %w", err) | ||
| // Validate message data build definition | ||
| if cfg.MessageData == nil { |
There was a problem hiding this comment.
Is this duplicated with ValidateMessageData
There was a problem hiding this comment.
Well, in fact, there should not be possible to start a Sentinel without MessageData configured... what would be the purpose of it? just bombarding the API?
It should return and error
| Generation int32 `json:"generation"` | ||
| Labels map[string]string `json:"labels"` | ||
| Status ResourceStatus `json:"status"` | ||
| OwnerReferences *OwnerReference `json:"owner_references,omitempty"` |
There was a problem hiding this comment.
We are mixing singular and plural here, should these be aligned?
There was a problem hiding this comment.
This one is tricky, the name itself comes from kubernetes
Dependent objects have a metadata.ownerReferences field that references their owner object. A valid owner reference consists of....
So, the field itself is owner_references meaning that it "references" to something... not that there are plural "references" to refer to.... 😵
There was a problem hiding this comment.
That is not confusing at all lol
rh-amarin
force-pushed
the
HYPERFLEET-353
branch
from
5b910a1 to
e8b9e5b
Compare
rh-amarin
force-pushed
the
HYPERFLEET-353
branch
from
e8b9e5b to
4742d15
Compare
|
@rh-amarin: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
5 participants
https://issues.redhat.com/browse/HYPERFLEET-643
Changes
internal/payload(new package)internal/client/client.gointernal/sentinel/sentinel.gointernal/config/config.goConfig examples (configs/)
Summary by CodeRabbit
New Features
Configuration Changes
Tests
Documentation