-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Check task attribute before use in sentry.add_tagging() #37143
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.
Can you add the test cased for the 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.
The change looks good, but it would be better to reproduce the bug in the unit tests and confirm that it is fixed with your change, in this case we can avoid breaking it again in the future.
661fe42
to
2b4b46c
Compare
@dirrao @hussein-awala I have added a test for this. I had to refactor the sentry module because the old And one more thing, I renamed |
2b4b46c
to
f880a38
Compare
e9c9e18
to
0efee2c
Compare
Needs rebase and resolve conflicts |
6604fde
to
6655f04
Compare
6655f04
to
bcd0a65
Compare
@LipuFei can you fix static checks? |
bcd0a65
to
3d53daa
Compare
3d53daa
to
83f528c
Compare
f8e7fea
to
25f348e
Compare
@eladkal Static checks have been fixed. 😃 |
Cool |
Hi @eladkal , this PR fixes the problem from another angle. Although the issue has been fixed, I think we can still keep this PR, because my changes make Sentry code testable. We can add some test coverage on the Sentry code. |
OK @potiuk reviewed the first PR so lets wait for his review |
That's just related - it's a bigger change that requires thorough -review -the original one was a quick fix. But yes - this one is more comprehensive (thouh I'd classify it as refactor not fix) This one is hard to review due to the way Git shows it. Could you please help a bit with the review @LipuFei by making some concise way of showing what has changed between the original and new code split out ? |
I think that would also help @eladkal and @hussein-awala to make such review. |
@potiuk I will split the commits so they are easier to review. |
25f348e
to
24f118c
Compare
@eladkal @potiuk @hussein-awala Here is a summary of the changes:
|
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.
NICE. I had to do some more git comit dance and --ignore-space-change
with diff helped . thanks for splitting it, that helped a bit to see that the second commit is indeed just a refactor.
…he#37143)" (apache#38519) This reverts commit 77d2fc7.
* Check task attribute before use in add_tagging * Refactor sentry and add tests --------- Co-authored-by: Lipu Fei <[email protected]>
…he#37143)" (apache#38519) This reverts commit 77d2fc7.
Check the the
TaskInstance.task
attribute before using it, becauseTaskInstance.task
may not be set.closes: #37142