RHINENG-26348: add skip notifications for platform events - #2286
Open
TenSt wants to merge 1 commit into
Open
Conversation
Reviewer's GuideAdds a skip_notifications flag to PlatformEvent and evaluator advisory notification flow, allowing advisory_account_data.notified to be marked without publishing Kafka notifications, and introduces tests covering JSON round-trip and skip-publish behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
publishNewAdvisoriesNotification, whennotificationsPublisheris nil you now return before callingmarkAdvisoriesNotified, which means advisories remain unmarked and can still flood later; consider either callingmarkAdvisoriesNotifiedin this branch or restoring the early nil check before doing the DB work so behavior is explicit. - Since
markAdvisoriesNotifiedencapsulates theadvisory_account_dataupdate, consider reusing it in any other paths that directly manipulatenotifiedto keep the update logic centralized and consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `publishNewAdvisoriesNotification`, when `notificationsPublisher` is nil you now return before calling `markAdvisoriesNotified`, which means advisories remain unmarked and can still flood later; consider either calling `markAdvisoriesNotified` in this branch or restoring the early nil check before doing the DB work so behavior is explicit.
- Since `markAdvisoriesNotified` encapsulates the `advisory_account_data` update, consider reusing it in any other paths that directly manipulate `notified` to keep the update logic centralized and consistent.
## Individual Comments
### Comment 1
<location path="base/mqueue/platform_event_test.go" line_range="27-34" />
<code_context>
+ assert.True(t, parsed.SkipNotifications)
+
+ // omitempty: false must not appear in JSON; fresh unmarshal defaults to false
+ event.SkipNotifications = false
+ data, err = sonic.Marshal(event)
+ assert.NoError(t, err)
+ assert.NotContains(t, string(data), "skip_notifications")
+
+ var parsedFalse PlatformEvent
+ assert.NoError(t, sonic.Unmarshal(data, &parsedFalse))
+ assert.False(t, parsedFalse.SkipNotifications)
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid brittle string-based assertion for omitting skip_notifications in JSON
The test currently checks `omitempty` by searching the marshaled JSON string for `"skip_notifications"`, which is fragile if formatting or encoding changes. Instead, unmarshal into a `map[string]any` and assert the key is absent:
```go
var m map[string]any
assert.NoError(t, sonic.Unmarshal(data, &m))
assert.NotContains(t, m, "skip_notifications")
```
This validates the JSON structure rather than relying on string content.
Suggested implementation:
```golang
// omitempty: false must not appear in JSON; fresh unmarshal defaults to false
+
+ event.SkipNotifications = false
+ data, err = sonic.Marshal(event)
+ assert.NoError(t, err)
+
+ var m map[string]any
+ assert.NoError(t, sonic.Unmarshal(data, &m))
+ assert.NotContains(t, m, "skip_notifications")
+
+ var parsedFalse PlatformEvent
+ assert.NoError(t, sonic.Unmarshal(data, &parsedFalse))
+ assert.False(t, parsedFalse.SkipNotifications)
```
If this test function currently ends immediately after the snippet you provided, no further changes are required. If there are other assertions that depend on the raw JSON string, consider converting them similarly to structure-based checks (using maps or strongly typed structs) to keep tests robust against formatting/serialization changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2286 +/- ##
==========================================
+ Coverage 59.27% 59.29% +0.01%
==========================================
Files 148 148
Lines 9488 9497 +9
==========================================
+ Hits 5624 5631 +7
- Misses 3276 3277 +1
- Partials 588 589 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TenSt
force-pushed
the
stepan/RHINENG-26348-add-skip-notifications-on-recalc
branch
from
July 30, 2026 17:25
af8c0bf to
19394a1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR:
skip_notificationsonPlatformEventfor recovery recalcadvisory_account_data.notifiedSummary by Sourcery
Add support for per-event skipping of instant advisory notifications while still marking advisories as notified.
New Features:
Enhancements:
Tests: