-
Notifications
You must be signed in to change notification settings - Fork 443
chore(ci): riotize appsec_integration_django #13868
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
|
Signed-off-by: Juanjo Alvarez <[email protected]>
Bootstrap import analysisComparison of import times between this PR and base. SummaryThe average import time from this PR is: 285 ± 9 ms. The average import time from base is: 290 ± 10 ms. The import time difference between this PR and base is: -4.5 ± 0.4 ms. Import time breakdownThe following import paths have shrunk:
|
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
BenchmarksBenchmark execution time: 2025-07-03 15:54:02 Comparing candidate commit a132902 in PR branch Found 0 performance improvements and 3 performance regressions! Performance is the same for 543 metrics, 4 unstable metrics. scenario:iastaspectsospath-ospathbasename_aspect
scenario:iastaspectsospath-ospathnormcase_aspect
scenario:iastaspectsospath-ospathsplitext_aspect
|
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
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.
question about a code change that should be unrelated, but otherwise lgtm
ddtrace/appsec/_iast/_handlers.py
Outdated
@@ -190,7 +190,8 @@ def _on_django_func_wrapped(fn_args, fn_kwargs, first_arg_expected_type, *_): | |||
return | |||
|
|||
http_req = fn_args[0] | |||
set_iast_request_endpoint(http_req.method, http_req.resolver_match.route) | |||
if http_req.resolver_match is not None: |
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.
necessary change?
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 is not needed (I was testing something because it failed with a None on that on CI, but the problem was because a wrong dependency), but let me test without it.
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.
Well, it was needed on riot, yup [raw log without this]. .
What wasn't needed was (if the current run finish correctly) the remote config env var.
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.
wait, but what changed? version of a dep being installed or something?
I'd expect this to be failing on hatch?
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Signed-off-by: Juanjo Alvarez <[email protected]>
Checklist
Reviewer Checklist