-
Notifications
You must be signed in to change notification settings - Fork 375
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
fix: prevent unintentional modification of arguments #2663
Conversation
Just a note but it feels like the |
Also it might be worth invoking Edit: forgot |
@coneill-enhance Thank you so much for the code changes. It makes total sense not to modify the original user object. As for the suggestions to change the internal code to make the API easier to work with are also legit. Going to check some internal documents regarding the internals of the API, and if there is room to make those adjustments, I will make sure to add them. |
A suggestion. It would be good to modify the test in If you do not have the bandwidth to tackle it, not a problem. Let me know. I would happily add them myself. Again, thanks for the contributions @coneill-enhance |
No problem. I'll add a test. |
Note:
|
Amazing work @coneill-enhance Yeah, the test is not related to your change. We are aware that we have some flaky tests on our end. Not worry I triggered another run. Once the CI is green I will merge the PR. Thanks for your contribution. |
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.
Thank you so much for your contribution.
@coneill-enhance. We have released 1.10.1 with the fix you summited. Thanks for your contribution. |
Thank you for the update, and thank you for providing the service, DataDog is incredibly useful. |
What does this PR do?
Fixes what appears to be a bug
Motivation
*1: In the original code:
Kit::Identity.set_user(trace, id: user_id, **user_options)
would be redundant as:id
exists inuser_options
and would replace the previously specified argumentAdditionally, if this was intended, then the following is equivalent and better:
or, not equivalent, but better still:
in all cases you still get a non-descriptive error if
user
isnil
Also checking for
user.id
here is redundant, and somewhat irrelevant as this is done again by:dd-trace-rb/lib/datadog/kit/identity.rb
Line 33 in 1d20aa8
So probably just go with: