-
Notifications
You must be signed in to change notification settings - Fork 467
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
[postoverflow] Reinject meta into overflown state #2502
[postoverflow] Reinject meta into overflown state #2502
Conversation
@LaurenceJJones: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
@LaurenceJJones: There are no area labels on this PR. You can add as many areas as you see fit.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
/kind enhancement |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2502 +/- ##
=======================================
Coverage 57.94% 57.95%
=======================================
Files 240 240
Lines 31039 31048 +9
=======================================
+ Hits 17986 17993 +7
- Misses 11418 11421 +3
+ Partials 1635 1634 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
lgtm
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.
lgtm
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.
lgtm
Converting to draft, as this implementation is pretty "mehhh" we should instead add a helper on the Alert or Overflown object to reinject it instead of magically just doing it as then users cant control the flow |
Closing for now due to concerns that re injecting the meta without any consideration from user perspective could cause confusion as we should control which alert in the post overflown is injected. |
#1337
This one is difficult to find an appropriate fix..... since Meta is converted to events on "overflow" anything set in meta in postoverflow stage is never carried beyond. This PR re injects all meta objects across all events, to make sure the user has it.
I am primarily looking for feedback as I couldnt really see any other property without creating a new one but we already have too many "meta" properties.