Skip to content
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

Fixed python 3 incompatibility in logging API implemenation of findCaller #5058

Merged
merged 4 commits into from
Oct 15, 2020

Conversation

nmaludy
Copy link
Member

@nmaludy nmaludy commented Oct 14, 2020

Closes #5057

See that issue for description of what the problem was. The fix was simply to ensure our implementation handles both the python 2 version and returns a 3-tuple, along with the python 3 version which returns a 4-tuple and is based on the code here: https://github.com/python/cpython/blob/3.9/Lib/logging/__init__.py#L1502-L1536

I've tested this change both my a python 2.7 (centos7) and python 3.6 (centos8 and ubuntu18) box and the logging modules are returned properly.

@nmaludy nmaludy added the bug label Oct 14, 2020
@nmaludy nmaludy requested review from blag, punkrokk and arm4b October 14, 2020 20:28
@nmaludy nmaludy self-assigned this Oct 14, 2020
@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Oct 14, 2020
@nmaludy nmaludy requested a review from amanda11 October 14, 2020 20:40
@m4dcoder m4dcoder added this to the 3.3.0 milestone Oct 14, 2020
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading and comparing with the python code, LGTM.
Though it seems there have been changes in 3.9 since you looked, so the line numbers have changed.

st2common/st2common/log.py Outdated Show resolved Hide resolved
@m4dcoder
Copy link
Contributor

@nmaludy Looks like there's an issue starting sensor container. The integration test in travis is failing consistently on both PRs.

@nmaludy
Copy link
Member Author

nmaludy commented Oct 15, 2020

@cognifloyd good catch, i had the right line numbers in the description of this PR, but must have had a bad copy/paste in the code itself.

@m4dcoder i re-ran the integration tests and they passed (i just committed @cognifloyd change so we'll see how they run this time), also note that i tested sensors explicitly since that is what @punkrokk reported and they definitely worked on my systems (centos7, centos8, ubuntu18)

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@m4dcoder
Copy link
Contributor

@nmaludy @cognifloyd LGTM Thanks guys!

@nmaludy nmaludy merged commit d3782db into master Oct 15, 2020
@nmaludy nmaludy deleted the hotfix/python3-logging-filename branch October 15, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ST2 logs writing "(unknown file)" in log prefix, instead of module name
3 participants