-
Notifications
You must be signed in to change notification settings - Fork 373
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
[APPSEC-55378] Extract processor context into separate file #4023
Conversation
BenchmarksBenchmark execution time: 2024-10-24 16:24:16 Comparing candidate commit a5e63c2 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 2 unstable metrics. |
b407e0d
to
5672c36
Compare
b98e1f6
to
8b44e52
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4023 +/- ##
==========================================
- Coverage 97.86% 97.84% -0.02%
==========================================
Files 1319 1321 +2
Lines 79322 79325 +3
Branches 3934 3934
==========================================
- Hits 77628 77616 -12
- Misses 1694 1709 +15 ☔ 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!
module AppSec | ||
class Processor | ||
class Context | ||
type event = untyped |
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.
I think event
is a ::Hash[Symbol, untyped] according to where it's used (e.g.: https://github.com/DataDog/dd-trace-rb/blob/master/lib/datadog/appsec/contrib/rack/gateway/watcher.rb#L47), I'm not sure it is more useful than declaring it as untyped though
type event = untyped | |
type event = ::Hash[Symbol, untyped] |
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.
Maybe, I'm not sure that I fully understand the code to change it now, we have events from the WAF
module too and so far they are always empty.
I think we can definitely change it as soon as we 100% sure.
8b44e52
to
a5e63c2
Compare
Change log entry
Not needed.
What does this PR do?
This is a preparation PR which extracts
Datadog::AppSec::Processor::Context
into a separate file. No changes (yet 😏) to the logic or implementation were done.Motivation:
We are going to work on those items soon and it makes sense to separate files.
Additional Notes:
None.
How to test the change?
Just CI.