-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use FQDN only for DNS and drop trailing dot for other operations #1255
Use FQDN only for DNS and drop trailing dot for other operations #1255
Conversation
Looks like the new version of Tox doesn't like our config file. Looking into it; easy fix would be to pin to 2.7.0 for now. |
Cool, let's see how this goes. |
Codecov Report
@@ Coverage Diff @@
## master #1255 +/- ##
======================================
Coverage 100% 100%
======================================
Files 21 21
Lines 1985 1989 +4
======================================
+ Hits 1985 1989 +4
Continue to review full report at Codecov.
|
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.
Cool, generally looks good! Some thoughts inline.
appveyor.yml
Outdated
@@ -57,7 +57,7 @@ install: | |||
# Upgrade to the latest version of pip to avoid it displaying warnings | |||
# about it being out of date. | |||
- pip install --disable-pip-version-check --user --upgrade pip wheel | |||
- pip install tox virtualenv | |||
- pip install tox==2.1.1 virtualenv |
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.
It would be nice if we could work out why tox is mad at us so we can remove these pins. Any idea?
(key, value) = header.split(b': ') | ||
if key.decode('ascii').startswith(u'Host'): | ||
received_host.append(value.decode('ascii')) | ||
|
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.
Might be nice to pull this header parsing logic out into a separate helper function. It's the kind of thing that might come up from time to 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.
Makes sense! Something like this?
def assert_header_received(self, received_headers, header_name, expected_value=None):
"""
Asserts that a header with the given name was received; if a value is passed to
`expected_value`, asserts that the header has that value.
"""
pass
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.
Yeah, sounds good to me!
urllib3/connection.py
Outdated
Setter for the `host` property. | ||
""" | ||
self._dns_host = value | ||
|
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.
Hrm, we might want to put a note here that points out that this assumes that only urllib3 code ever sets this property.
More generally, I wonder if we should just remove the setter and change the __init__
to do the right thing. What are your thoughts on that?
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.
Makes sense to me.
I would tend towards not changing __init__
; due to the way it's structured in httplib, we'd essentially have to copy out a chunk of stdlib code (not a huge chunk, sure, but a chunk) and make one small tweak; this is a relatively small change in comparison.
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.
Fair enough. 😄
Tox looks to be broken on parsing |
Did that stop being valid syntax? |
I don't think so, I suspect it's a regression. They've had other regressions in the past involving references to other sections like this |
Looks like this is fixed now, so I'll remove the pins and check again. |
Okay, I think the single Appveyor fail is spurious, but I don't see a way to restart the build without pushing another commit, so that can wait until @Lukasa okays the test method I suggested. The GAE failure on Travis is pretty consistent though; @jonparrott, any thoughts on that as the resident GAE expert? |
62b9e1c
to
88bcf98
Compare
Rebased and squashed from 62b9e1c. |
@Lukasa, it looks like Travis is currently experiencing issues with their macOS builds; if you want to give a runthrough and a final 👍, I can merge once those builds complete. |
88bcf98
to
6fa796a
Compare
Rebased from 88bcf98. |
Gentle ping to @Lukasa; please ignore if you're taking a break from OSS. @sigmavirus24, can you provide a review in case @Lukasa is unavailable? |
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 tiny note and then we're good to go.
dummyserver/testcase.py
Outdated
received_headers, | ||
header_name, | ||
expected_value=None | ||
): |
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.
I'm not wild about this indentation. 😉 Mind matching it up with the rest of the project?
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.
Sure! Another project I'm involved with had linting set up to whine if the parameters line up with the first line of the function, and that's why I did it here, but I agree it looks skeevy.
12d98c4
to
36bf9cc
Compare
@Lukasa, finally addressed that note. |
Looks like we need to add some pins to some of our dependencies for 2.6. |
36bf9cc
to
050a5f6
Compare
Rebased from 36bf9cc to pick up test changes. |
@Lukasa, can you do a final run-through on this? I believe it should be set to merge once we have a green light from Travis (test issues were resolved elsewhere). |
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.
LGTM, ship it
050a5f6
to
68f3475
Compare
…tificate Use FQDN only for DNS and drop trailing dot for other operations
Fixes #1254.