-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Support Python 3 #2831
Support Python 3 #2831
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2831 +/- ##
==========================================
+ Coverage 84.65% 88.37% +3.72%
==========================================
Files 660 4 -656
Lines 37759 86 -37673
Branches 4535 8 -4527
==========================================
- Hits 31963 76 -31887
+ Misses 4462 6 -4456
+ Partials 1334 4 -1330 |
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.
One comment, otherwise looks great to me, thanks!
statsd/tests/test_statsd.py
Outdated
# The only good way to mock this would be to duplicate the logic in the check | ||
# which doesn't make a very good test. | ||
# Instead, just add a sleep here since the container is pretty quick to spin up | ||
time.sleep(10) |
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.
time.sleep(10) | |
log_patterns=['server is up'] |
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.
Looks like the container emits the above log line that we can wait for instead of hardcoding a time.
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.
Looks good, thanks!
What does this PR do?
Make Statsd support Python 3
Motivation
🐍 🚂
Review checklist
no-changelog
label attachedAdditional Notes
There's a few things to note:
This check has extensive string handling. I have done my best to ensure that, I believe, it is handled consistently between the two versions. But it's complex enough that I am not 100% positive I didn't accidentally introduce a bug.
In Python 2 we used the URL checker to ensure that the docker container spun up. However, it did not really do that. It relied on a bug in the implementation.
There's no good way to do this for a raw tcp connection (instead of an http connection) without replicating the tcp handling logic we have in the check. So, instead, I've just put a sleep in there. This docker container is simple enough that it should always spin up in time.
However, it does introduce a source of uncertainty that is problematic. If someone has a thought on how to better to do this, I'm open to hearing it.