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

Adds DASH and PORT env vars #1134

Merged
merged 2 commits into from
Mar 20, 2020
Merged

Adds DASH and PORT env vars #1134

merged 2 commits into from
Mar 20, 2020

Conversation

OwenMatsuda
Copy link
Contributor

This PR adds the ability to set a DASH_HOST environment variable to change the host value when running the server.

@alexcjohnson
Copy link
Collaborator

Alright, after way too much discussion with @josegonzalez and @rpkyle I'm on board with this change. Looks like you just have to adjust the usage re the test failure:

dash/dash.py:1913:13: W1508: os.getenv default type is builtins.int. Expected str or None. (invalid-envvar-default)

And let's give it some helpful behavior if you fail to provide an int or a string that can be coerced to an int. Then we just need a CHANGELOG entry for the new options and I think this will be ready to 💃

@OwenMatsuda
Copy link
Contributor Author

And let's give it some helpful behavior if you fail to provide an int or a string that can be coerced to an int.

@alexcjohnson What kind of helpful behavior do you think? Should it just default to the 8050 port, or should it throw an error?

@alexcjohnson
Copy link
Collaborator

Throw an error like "Expecting an integer from 1 to 65535, found {}".format(repr(port)) (only including the range if you actually verify that, which would be a nice touch)

@OwenMatsuda OwenMatsuda force-pushed the host-port-env branch 3 times, most recently from 846f445 to da3f4ff Compare March 3, 2020 20:18
@OwenMatsuda
Copy link
Contributor Author

I think this is ready for review. My only potential concern is that for port, when the user inputs it as a parameter within app.runserver(port=8050), port is an int. However, when grabbing it from an environment variable, port is a string that is later coerced into an int within the try/except block.

:param port: Port used to serve the application
env: ``PORT``
:type port: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just mark port here as either an int or a string containing an int, and we should be covered re: data types. Your coercion and try/except below looks great!

Can you please add a couple of tests of the failure cases, and maybe one of setting an integer string port and verifying that an app shows up there? These can probably all go in https://github.com/plotly/dash/blob/dev/tests/unit/test_configs.py

The failures can just be something like:

app = Dash()
app.layout = html.Div()
with pytest.raises(...) as excinfo:
    app.run_server(port="garbage")
assert excinfo.value == ...

And the pass case can be something like:

app = Dash()
app.layout = html.Div("hi", id="out")
dash_duo.run_server(app, port="12345")
assert dash_duo.server_url == "https://127.0.0.1:12345"
dash_duo.wait_for_text_to_equal("out", "hi")

@OwenMatsuda
Copy link
Contributor Author

I'm trying to use pytest's monkeypatch to mock env vars, but I'm running into a bug: pytest-dev/pytest#6858

@alexcjohnson
Copy link
Collaborator

There's a fixture empty_environ at the top of the test_configs file - does that help?

@OwenMatsuda
Copy link
Contributor Author

I saw that, but that would mean adding the env vars HOST AND PORT to dash/_configs.py. At first, I thought that would make more sense than the setup I have now, but then I noticed a problem:

    env = load_dash_env_vars().get('DASH_{}'.format(name.upper()))
    if env is None:
        return default

Unless we wanted to add another function specific to HOST and PORT, we would have to rename them to DASH_HOST, and DASH_PORT, respectively, which would be misaligned with plotly/dashR#167.

Along those same lines, I was wondering what the "parity" with Dash for Python was since the HOST and PORT didn't previously exist.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Beautiful. 💃

@alexcjohnson alexcjohnson merged commit dc718fd into dev Mar 20, 2020
@alexcjohnson alexcjohnson deleted the host-port-env branch March 20, 2020 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants