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

use SERVER_NAME to set host and port in app.run() #2152

Merged
merged 5 commits into from Jan 17, 2017

Conversation

@r0fls
Copy link
Contributor

commented Jan 16, 2017

Addresses #2109

server_name = self.config.get("SERVER_NAME", None)
if server_name:
_host, _port = servername.split(':', 1)
host, port = host or _host, port or int(_port)

This comment has been minimized.

Copy link
@davidism

davidism Jan 16, 2017

Member

Don't use tuple unpacking here.

This comment has been minimized.

Copy link
@davidism

davidism Jan 16, 2017

Member

This fails if there was no port in SERVER_NAME.

This comment has been minimized.

Copy link
@jeffwidman

jeffwidman Jan 16, 2017

Member

Good catch. I forgot about that. I like the way this is structured though, reads much cleaner to my eyes than the previous logic.

@davidism davidism changed the title use SERVER_CONFIG in app run() use SERVER_NAME to set host and port in app.run() Jan 16, 2017

@r0fls r0fls force-pushed the r0fls:2109 branch from ed4cff5 to 823d6e0 Jan 16, 2017

_port = 5000
server_name = self.config.get("SERVER_NAME", None)
if server_name:
_host, _port = servername.split(':', 1)

This comment has been minimized.

Copy link
@jeffwidman

jeffwidman Jan 16, 2017

Member

server_name rather than servername?

@r0fls

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2017

Yikes, thanks @jeffwidman. Any pointers on tests we could write to catch some of these types of situations?

_port = 5000
server_name = self.config.get("SERVER_NAME", None)
if server_name:
_host, _port = server_name.split(':', 1)

This comment has been minimized.

Copy link
@davidism

davidism Jan 16, 2017

Member

This still fails if there's no port.

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Jan 16, 2017

Member

https://docs.python.org/2/library/stdtypes.html#str.partition is somewhat useful for cases like this one

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

Is there a reason you didn't include the "set SEVER_NAME from run" part of the issue? It would require some more thought because you can't set the config to some values such as 0.0.0.0 (even though you shouldn't be running with that anyway).

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

Posted this in the linked issue, but note that this won't and can't apply to the flask run command, which is the recommended way to run the dev server.

@jeffwidman

This comment has been minimized.

Copy link
Member

commented Jan 16, 2017

I haven't looked at the original issue in depth, but you got me curious on how to use partition properly... played with it and looks like something like this would properly handle the double fallback from app.run to SERVER_NAME to the built-in default. And it does it separately for both host and port values so you can specify one or the other or both:

_host = '127.0.0.1'
_port = 5000
server_name = self.config.get("SERVER_NAME")
if server_name:
    _sn_host, _, _sn_port = server_name.partition(':')
host = host or _sn_host or _host
port = int(port or _sn_port or _port)

@r0fls Feel free to use as much or as little of this as you want. I just really liked the clean/linear way your original implementation walked through the configs, so I'd hate to see your solution get too cluttered up...


_host = '127.0.0.1'
_port = 5000
server_name = self.config.get("SERVER_NAME", None)

This comment has been minimized.

Copy link
@jeffwidman

jeffwidman Jan 17, 2017

Member

None is the default fallback, no need to specify it

This comment has been minimized.

Copy link
@r0fls

r0fls Jan 17, 2017

Author Contributor

I always forget that.

server_name = self.config.get("SERVER_NAME")
if server_name:
sn_host, _, sn_port = server_name.partition(':')
host = host or sn_host or _host

This comment has been minimized.

Copy link
@davidism

davidism Jan 17, 2017

Member

This will fail if SERVER_NAME wasn't set, since sn_host won't be set.

@r0fls

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

Should I rebase to 1 commit at this point, or does that not matter?

@r0fls r0fls force-pushed the r0fls:2109 branch from 37ca487 to 3a48831 Jan 17, 2017

@jeffwidman
Copy link
Member

left a comment

👍

(Of course, I did say LGTM when it was broken, so take my review with a grain of salt)

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Not sure why the Python 2.6 tests started failing, but it seems unrelated.

@r0fls

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2017

Ya, it looks to be a message saying 2.6 is no longer supported or maintained by the core devs.

@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

That is true but Flask still supports it. However, it appears that coverage.py no longer does.

@davidism davidism merged commit 1636a4c into pallets:master Jan 17, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@untitaker

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

This needs a testcase and changelog.

davidism added a commit that referenced this pull request Jan 17, 2017

@davidism

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Done.

Re: 2.6, it was fixed in Coverage earlier today, tests pass again. https://coverage.readthedocs.io/en/coverage-4.3.4/changes.html#version-4-3-4-2017-01-17

@john-bodley

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@davidism I was wondering whether the docstring for the host param should now reflect the updated behavior, i.e. similar to port,

... or the host defined in the SERVER_NAME config variable if present.

@jeffwidman

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

That is probably a good idea, do you want to open a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.