Make sure to reset AppSec settings when calling Datadog.configuration.reset!
#2910
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2910 +/- ##
==========================================
- Coverage 98.11% 97.98% -0.13%
==========================================
Files 1266 1274 +8
Lines 70152 70319 +167
Branches 3187 3229 +42
==========================================
+ Hits 68827 68903 +76
- Misses 1325 1416 +91 see 60 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
GustavoCaso
force-pushed
the
fix-possible-leaky-specs
branch
from
0594c9b
to
2974efc
Compare
| Datadog.configuration.reset! | ||
| Datadog.configuration.appsec.send(:reset!) |
There was a problem hiding this comment.
This is... weird. Should Datadog.configuration.reset! reset all of the configuration, including appsec?
There was a problem hiding this comment.
This is one of the reasons I mentioned to you that I want to write an RFC for having a core configuration. AppSec has its own way of adding extensions.
So Ideally yes, but at the moment
There was a problem hiding this comment.
Indeed appsec configuration is... in flow I guess. Can I convince you to add Datadog.configuration.appsec.send(:reset!) to Datadog::Configuration#reset!?
While doing that doesn't solve tech debt, it seems reasonable to do until we can do a better solution.
(But, on the other hand, if I can't convince you, and just to clarify, I don't think it's a blocking issue, happy to approve either way, if we really have to )
github-actions
bot
added
core
GustavoCaso
changed the title
Datadog.configuration.reset!
| @@ -91,6 +91,7 @@ def dig(*options) | |||
|
|
|||
| def reset! | |||
| reset_options! | |||
| Datadog::AppSec.settings.send(:reset!) | |||
There was a problem hiding this comment.
Thanks for doing this change. Just in case, let me ask -- are we sure we can always call this method on Datadog::Appsec?
E.g. I seem to remember a few cases where the constants were not loaded in some cases and we need to add a few defined?(...) checks.
There was a problem hiding this comment.
The only case I can think of is when people follow the CI documentation since they only require datadog/ci.
I'm not sure how many times people would call reset! on those cases, but I will add the defined? just in case
GustavoCaso
force-pushed
the
fix-possible-leaky-specs
branch
from
599b534
to
fa033c3
Compare
What does this PR do?
Trying to fix those annoying leaky specs for AppSec regarding if is runing or not 😡
Motivation
Additional Notes
How to test the change?