-
-
Notifications
You must be signed in to change notification settings - Fork 36
FIX: missing ip settings from SDK package #957
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
Conversation
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.
Awesome work @lucas-zimerman 🚀
The changes LGTM. Thank you adding unit test and test data in the comments.
Added a couple of questions but nothing blocking.
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.
Thanks for fixing this! Looks good to me!
I'm thinking that the SDK might still set user.ip_address: '{{auto}}'
in case setDefaultPii: true
. In case it does, we could still still clean this up but it's not the end of the world either way. sdk.settings.infer_ip
overrules the other field in both cases.
I have removed the auto ip here 49d6b67 |
PR test error is unrelated to the added code and will be fixed on another PR |
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.
thanks!
Based on getsentry/sentry-javascript#17364
original context;
Instead of bumping o V10, the fix was patched on the current release.