Conversation
|
|
678bc2d to
7615cdb
Compare
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 4d60a43 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
7615cdb to
c64707f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c64707fae2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @scenarios.go_proxies_default | ||
| @scenarios.default | ||
| class Test_StandardTagsNetworkClientIp: |
There was a problem hiding this comment.
Gate network client IP tests by supported tracer versions
Adding this class enables it by default in DEFAULT and GO_PROXIES_DEFAULT for any library/version that lacks an explicit manifest rule, because unspecified tests are treated as enabled (see docs/edit/manifest.md). That creates a regression for older tracer versions that still do not emit http.client_ip/network.client.ip (the existing Test_StandardTagsClientIp is version-gated per language, e.g. in manifests/nodejs.yml and manifests/java.yml), so test_network_client_ip_with_attack can start failing in release-version runs. Please add class/method manifest gating for this new class with the same support floor as client-ip tagging.
Useful? React with 👍 / 👎.
| assert network_client_ip != self.PUBLIC_IP | ||
| # http.client_ip resolves proxy headers, while network.client.ip does not, so both should be different here. | ||
| http_client_ip = meta.get("http.client_ip") | ||
| assert http_client_ip |
There was a problem hiding this comment.
also add http_client_ip == self.PUBLIC_IP
the rest looks good to me
There was a problem hiding this comment.
I was intentionally not doing it since it's covered by the http.client_ip tests, but it doesn't hurt. I'll do it.
c64707f to
04fab3f
Compare
nccatoni
left a comment
There was a problem hiding this comment.
LGTM for @DataDog/system-tests-core but you should get an approval from someone familiar with the feature
04fab3f to
0fcf185
Compare
0fcf185 to
4d60a43
Compare
Motivation
APPSEC-62223
Changes
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present