Replaced pycurl with urllib3 in appservice web log stream #907
Conversation
|
Hi @haocs, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
|
Related issue: #905 @derekbekoe @yugangw-msft Please help review this PR, thanks! |
haocs
changed the title
derekbekoe
left a comment
derekbekoe
left a comment
There was a problem hiding this comment.
LGTM but would check with @yugangw-msft to make sure it behaves as intended. One less native dependency to deal with 👍
There was a problem hiding this comment.
Add from __future__ import print_function as the very first import.
Pylint is complaining in 2.7 because of this print statement.
haocs
changed the title
| c.perform() | ||
| import urllib3 | ||
|
|
||
| urllib3.disable_warnings() |
There was a problem hiding this comment.
Why do you need to disable warnings?
There was a problem hiding this comment.
urllibs generates SSL warnings that has nothing to do with webapp logs. So it's better to suppress those potential warnings.
There was a problem hiding this comment.
Which SSL warnings? Suppressing potential security related warnings is scary to me...
| urllib3.disable_warnings() | ||
| http = urllib3.PoolManager() | ||
| headers = urllib3.util.make_headers(basic_auth='{0}:{1}'.format(user_name, password)) | ||
| r = http.request( |
There was a problem hiding this comment.
Don't you need to release the request at some point?
There was a problem hiding this comment.
stream request will be closed when client reads the end of content or server terminates it.
There was a problem hiding this comment.
Actually, I was wrong here. Added releasing connection here.
| import urllib3 | ||
|
|
||
| urllib3.disable_warnings() | ||
| http = urllib3.PoolManager() |
There was a problem hiding this comment.
We should make sure that we honor the "disable cert check" settings in order to support the use of network monitor/capture...
There was a problem hiding this comment.
I agree. But in this case, do we expect our users take any action if they see those warnings?
|
@johanste @derekbekoe Please help review again, Thanks |
| 'azure-cli-core', | ||
| 'azure-mgmt-web==0.30.0rc6', | ||
| 'pycurl' | ||
| 'urllib3==1.17' |
There was a problem hiding this comment.
Think we should remove the pinned version here unless there's a requirement that we use this specific version?
|
I'm fine with pinning to 1.16 for now |
mayurid
changed the title
|
Is this being merged in this morning? |
derekbekoe
changed the title
5 participants
No description provided.