-
Notifications
You must be signed in to change notification settings - Fork 50
Use the existing and configured env indicator #476
Conversation
@@ -39,7 +39,7 @@ | |||
# SECURITY WARNING: don't run with debug turned on in production! | |||
DEBUG = config("DJANGO_DEBUG_ENABLED", default=False, cast=bool) | |||
|
|||
PYTHON_ENV = config("PYTHON_ENV", default="production") | |||
ENVIRONMENT = config("ENVIRONMENT", default="local") |
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.
Why did it change to "local"?
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's the default that is used by the ingestion_server when it accesses this same variable:
environment = config("ENVIRONMENT", default="local") |
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 think we should keep "production" and change to "staging" or the corresponding string in the staging server. "local" doesn't seem very useful as everything is local to its own machine.
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.
Yea I agree that naming convention isn't too useful! 😅 I think dev
or staging
makes sense. A lot of the environment checks in the ingestion server are checking explicitly if in prod (environment == "prod"
) so it hasn't mattered much what not prod is.
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 agree local
is ambiguous.
In my mind (and indeed I've seen this in practice) dev
is local development, staging
is the pre-production deployment, prod
is production, and test
is when tests are running regardless of environment. Also variations on those strings exist. Node (and most JavaScript projects like React and Vue) uses the full development
and production
specifically for example, so I like the consistency of using that for the less-opinionated Python environment.
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.
Looks good! We can follow up with a future PR to change "local" to something clearer 🙂
@@ -39,7 +39,7 @@ | |||
# SECURITY WARNING: don't run with debug turned on in production! | |||
DEBUG = config("DJANGO_DEBUG_ENABLED", default=False, cast=bool) | |||
|
|||
PYTHON_ENV = config("PYTHON_ENV", default="production") | |||
ENVIRONMENT = config("ENVIRONMENT", default="local") |
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.
Yea I agree that naming convention isn't too useful! 😅 I think dev
or staging
makes sense. A lot of the environment checks in the ingestion server are checking explicitly if in prod (environment == "prod"
) so it hasn't mattered much what not prod is.
Fixes
Fixes #475 by @sarayourfriend
Description
Replace
PYTHON_ENV
with the already usedENVIRONMENT
.Testing Instructions
Checkout the branch and update your local instance (
just recreate && just init
should do it but will wipe local data) and verify the API runs.Unit tests should also pass of course.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin