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

honor port env variable for python web server examples #338

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ajshedivy
Copy link
Member

This should make all python web server examples compatible with cluster mode in ServiceCommander.

@ajshedivy ajshedivy force-pushed the fix/python-env-port branch 2 times, most recently from 7b1db2d to 3ea9654 Compare September 12, 2022 20:07
@@ -46,4 +47,4 @@ def cmd_toolkit():
return render_template('cmd.html', data=data)

app.debug = True
app.run(host='0.0.0.0', port=9000,)
app.run(host='0.0.0.0', port=os.getenv('PORT', '9000'),)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think Flask allows the port to be specified as a string. You'd need to wrap this: int(os.getenv('PORT', 9000))

I'm not sure if bottle has the same limitation, but I suspect so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add the int wrapper to the flask example. I have tested the active-jobs-dashboard locally and with ServiceCommander, and doesn't seem to enforce PORT being an int

@kadler
Copy link
Member

kadler commented Sep 19, 2022

Realizing that these examples really could use some sprucing up to show best practices. The dashboard example should be converted to use Flask and the bottle example should probably be removed (or a disclaimer that Flask is much more widely used).

@ajshedivy
Copy link
Member Author

moving #339 to this pull request

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