-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
changes to enable module use #787
Conversation
server/api/src/settings.py
Outdated
@@ -10,8 +11,8 @@ class Version: | |||
|
|||
|
|||
class Server: | |||
HOST = '0.0.0.0' | |||
PORT = env('PORT', to.INT) | |||
HOST = os.getenv('HOST', '0.0.0.0') |
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.
Can we do HOST = env('HOST')
here, and then add HOST=0.0.0.0
before PORT
in the .env.example
file? And also remove import os
from the top of this file.
I've found default environment variables to be convenient but ultimately counterproductive. If we're explicit about everything we expect the environment to to define -- rather than relying on defaults -- then it's easier to manage the production environment. You can just check the .env
file to figure out what's happening instead of digging around in code.
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 didn't need the casting logic so was using the getenv approach, but makes sense to be consitent.
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.
Thanks! I'm just adding a tiny commit moving the host default (0.0.0.0) into the .env.example file
server/api/src/settings.py
Outdated
HOST = '0.0.0.0' | ||
PORT = env('PORT', to.INT) | ||
HOST = os.getenv('HOST', '0.0.0.0') | ||
PORT = env('PORT', to.INT, ) |
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.
is that second comma necessary?
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.
must've added that accidentally!
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.
Hey this looks great! Just a few requests regarding the settings file.
addressing PR comments. thanks for the feedback! |
So this change is to allow the API to be called as a module IN ADDITION to being run from bin/api_start. This means I don't need to use Docker to run the API. With this change I can debug the API locally (only running Postgres in Docker). I have some vscode settings files and a local dev.env file that sets PYTHONPATH to make this all work (VS Code is a little fiddly) which I can include in a future commit if other devs use VS Code.
I also included a baseline pipfile and pipfile.lock. I use pipenv for my projects. I've had good luck with it in the past, but would be happy to try poetry too. It might be good to standardize on some package management tool.
None of these changes should impact the current build process at all.
I didn't have an issue for these changes, so just included 'feat' in the branch name.
dev
branchAny questions? See the getting started guide