-
Notifications
You must be signed in to change notification settings - Fork 9
HYPERFLEET-353: Update sentinel published message #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HYPERFLEET-353: Update sentinel published message #35
Conversation
WalkthroughThis change updates field naming conventions across configuration templates and event payloads, replacing Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Key areas requiring attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
configs/dev-example.yaml(1 hunks)internal/client/client.go(1 hunks)internal/client/client_test.go(2 hunks)internal/sentinel/sentinel.go(1 hunks)
🔇 Additional comments (3)
configs/dev-example.yaml (1)
45-46: LGTM! Config template updated to match new field names.The message_data template keys have been correctly renamed to align with the CloudEvent payload changes in sentinel.go (
resource_id→id,resource_type→kind). The template structure is consistent and maintains the same functionality with the new naming convention.internal/client/client.go (1)
360-360: LGTM! Generation field now properly sourced from API response.The Generation field for NodePools is now correctly populated from
item.Generationinstead of being hardcoded to 0. This fixes the test error mentioned in the PR description and aligns NodePool behavior with Cluster resource handling (line 259). The change ensures accurate resource version tracking for NodePools.internal/client/client_test.go (1)
549-549: LGTM! Test updated to reflect generation field sourcing from API.The test mock response now includes a
generationfield with value 3, and the assertion correctly expectsGeneration == 3instead of the previous hardcoded 0. This properly validates the fix in client.go where NodePool generation is now sourced from the API response.Also applies to: 593-595
| if err := event.SetData(cloudevents.ApplicationJSON, map[string]interface{}{ | ||
| "resourceType": resource.Kind, | ||
| "resourceId": resource.ID, | ||
| "reason": decision.Reason, | ||
| "kind": resource.Kind, | ||
| "id": resource.ID, | ||
| "generation": resource.Generation, | ||
| "href": resource.Href, | ||
| "reason": decision.Reason, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'resourceType' usage in codebase ==="
rg -nP --type=go 'resourceType' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for 'resourceId' usage in codebase ==="
rg -nP --type=go 'resourceId' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for asyncapi.yaml file ==="
fd -t f 'asyncapi.yaml' 2>/dev/null || echo "No asyncapi.yaml found"
echo -e "\n=== Searching for CloudEvent test files ==="
fd -t f -e 'test' 'event|cloud' 2>/dev/null | head -20
echo -e "\n=== Checking if there are any event-related test files ==="
find . -type f \( -name "*event*test*" -o -name "*test*event*" \) 2>/dev/null | head -20
echo -e "\n=== Searching for SetData references to verify payload structure changes ==="
rg -nP --type=go 'SetData.*ApplicationJSON' -C3 2>/dev/null | head -40Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 8258
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'resourceType' usage in codebase ==="
rg -nP --type=go 'resourceType' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for 'resourceId' usage in codebase ==="
rg -nP --type=go 'resourceId' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for asyncapi.yaml file ==="
fd -t f 'asyncapi.yaml' 2>/dev/null || echo "No asyncapi.yaml found"
echo -e "\n=== Searching for CloudEvent test files ==="
fd -t f -e 'test' 'event|cloud' 2>/dev/null | head -20
echo -e "\n=== Checking if there are any event-related test files ==="
find . -type f \( -name "*event*test*" -o -name "*test*event*" \) 2>/dev/null | head -20
echo -e "\n=== Searching for SetData references to verify payload structure changes ==="
rg -nP --type=go 'SetData.*ApplicationJSON' -C3 2>/dev/null | head -40Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 8258
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'resourceType' usage in codebase ==="
rg -nP --type=go 'resourceType' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for 'resourceId' usage in codebase ==="
rg -nP --type=go 'resourceId' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for asyncapi.yaml file ==="
fd -t f 'asyncapi.yaml' 2>/dev/null || echo "No asyncapi.yaml found"
echo -e "\n=== Checking if there are any event-related test files ==="
find . -type f \( -name "*event*test*" -o -name "*test*event*" \) -o -name "*sentinel*test*" 2>/dev/null | head -20
echo -e "\n=== Searching for SetData references to verify payload structure changes ==="
rg -nP --type=go 'SetData.*ApplicationJSON' -C3 2>/dev/null | head -60Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 8250
🏁 Script executed:
#!/bin/bash
echo "=== Searching for 'resourceType' usage in codebase ==="
rg -nP --type=go 'resourceType' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for 'resourceId' usage in codebase ==="
rg -nP --type=go 'resourceId' -g '!internal/sentinel/sentinel.go' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for asyncapi.yaml file ==="
fd -t f 'asyncapi.yaml' 2>/dev/null || echo "No asyncapi.yaml found"
echo -e "\n=== Checking sentinel-related test files ==="
find . -type f -name "*sentinel*" 2>/dev/null
echo -e "\n=== Searching for SetData CloudEvent payload calls ==="
rg -nP --type=go 'SetData' -C2 2>/dev/null | head -80Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 8130
🏁 Script executed:
cat -n internal/sentinel/sentinel_test.goRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 13883
Tests must verify the new CloudEvent data payload structure and ensure backward compatibility or migration path.
The CloudEvent payload has been updated with field renames and new fields (generation, href). While the test suite includes event publishing tests, they verify only the CloudEvent envelope (type, source, specVersion) but do not validate the data payload structure with the new fields (kind, id, generation, href, reason).
Please ensure:
- Add test assertions that verify the CloudEvent data payload contains the new field names and their expected values
- Confirm no asyncapi.yaml spec exists in this repository or update tests to validate against the spec if external
- Document the breaking change for downstream consumers and provide a migration timeline or support plan
🤖 Prompt for AI Agents
In internal/sentinel/sentinel.go around lines 112 to 117 the CloudEvent data
payload was changed to include renamed/new fields (kind, id, generation, href,
reason) but tests only validate the event envelope; update the test suite to
assert the CloudEvent data payload contains each of these keys with the expected
values (kind == resource.Kind, id == resource.ID, generation ==
resource.Generation, href == resource.Href, reason == decision.Reason), add
negative/backwards compatibility checks if previous field names were removed or
map both old→new if supporting migration, verify whether an asyncapi.yaml exists
in the repo and if so update or add tests to validate the payload against that
spec (or add a note that none exists), and add a short test-oriented changelog
entry (or documentation file) noting the breaking change and a migration
timeline/support plan for downstream consumers.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
356a2a9
into
openshift-hyperfleet:main
Change the current hardcoded Sentinel message format to adhere to the async spec
This change allows to deploy a working CLM solution for clusters, for nodePools the ticket HYPERFLEET-353 has to be addressed for more flexibility to create the message payload.
This PR also fixes an error running
make testsince no generation was present on NodePool API responses.Summary by CodeRabbit
Bug Fixes
Updates
resource_id→id,resource_type→kindgenerationandhreffields to event payloads✏️ Tip: You can customize this high-level summary in your review settings.