Use POSTGRES_* and REDIS_URL as fallback #4811
Conversation
foarsitter
changed the title
foarsitter
force-pushed
the
config_postgres_celery_vars
branch
2 times, most recently
from
0ae5d90 to
9c70d0a
Compare
foarsitter
force-pushed
the
config_postgres_celery_vars
branch
from
9c70d0a to
19cf1cd
Compare
There was a problem hiding this comment.
Looking at the issue about hybrid dev, I'd like to ask a few questions, in the hope that we can maybe achieve the same outcome, without the try/except:
- Why is it easier to set 5 env variables (
POSTGRES_...) than 1 (DATABASE_URL) in your hybrid dev mode (without Docker)? - The
POSTGRES_...vars are set in Docker, so do we actually need theDATABASE_URL? - Could we instead replace the entrypoint script by an off-the-shelf solution as suggested in Why prefer python than bash for wait the service start #4539?
Then perhaps we could always use the POSTGRES_... variables instead?
| except environ.ImproperlyConfigured: | ||
| DATABASES = { | ||
| "default": { | ||
| "ENGINE": "django.db.backends.postgresql", | ||
| "NAME": env.str("POSTGRES_DB"), | ||
| "USER": env.str("POSTGRES_USER"), | ||
| "PASSWORD": env.str("POSTGRES_PASSWORD"), | ||
| "HOST": env.str("POSTGRES_HOST"), | ||
| "PORT": env.str("POSTGRES_PORT"), | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not a big fan of this except 🤔 I can't quite really put a finger on why, though (sorry, that's unhelpful)
There was a problem hiding this comment.
I think it's because in case of problems, it becomes harder to figure out where the DB credentials are coming from. Do I have a DATABASE_URL set? Do I have anything missing from the required POSTGRES_... env?
Sure, it's only one more place to check, but it can be really confusing when your app won't conect to the DB
There was a problem hiding this comment.
Relaying on exceptions here is a lazy way of doing things I suppose. When a faulty url is given it would try to use the postgres vars, so I agree on your reluctance and it should be improved.
| try: | ||
| CELERY_BROKER_URL = env("CELERY_BROKER_URL") | ||
| except environ.ImproperlyConfigured: | ||
| CELERY_BROKER_URL = env("REDIS_URL") |
There was a problem hiding this comment.
When I look at this, I wonder why we need CELERY_BROKER_URL in the first place 🤔
There was a problem hiding this comment.
For a project, in the time that Heroku had free redis instances, I used a three instances: a cache backend, celery broker and result backend. But yeah, we can get rid of it since it doesn't add any value.
When I'm talking about hybrid I' talking about a local interpreter with docker services. In a non docker setup It wouldn't matter to me to set 5 or just 1 composed out of 5.
If we get rid of
We can put Sweet, I like where we are going! |
|
|
This PR extends the base configuration in two ways:
DATABASE_URLis missing it is usingPOSTGRES_*variables for configuring the database.CELERY_BROKER_URLdefaults toREDIS_URLWhen using #4810 the
POSTGRES_*andREDIS_URLvariables are present but I'm not using theentrypointscript. This modification gives the possibility to run withoutentrypointand without further modifications by using the variables already present.