search

kitware-resonant / django-composed-configuration

Turnkey Django settings for data management applications.
Apache License 2.0
9 stars 2 forks source link

Add 'django' to ALLOWED_HOSTS in dev config #154

Closed zachmullen closed 3 years ago

zachmullen commented 3 years ago

This is to support other services communicating with django over HTTP in a docker-compose development environment.

I realize this might not be in scope here. Hopefully the maintainers can recommend a better alternative to achieve this if this isn't the right place.

zachmullen commented 3 years ago

It also occurs to me that it may be desirable to condition this on _is_docker(), but I'm not sure.

brianhelba commented 3 years ago

I'd prefer that this not hardcode in the hostname docker, since that's up to the Docker Compose config, which allowed to vary and is downstream of this library. Ideally, that could be auto-detected when _is_docker(), but I'm not sure whether such detection is possible from inside the container.

Alternatively, we could just set ALLOWED_HOSTS to '*', but since forming absolute URLs depends on the Host headers, it's too important to allow a development environment to diverge from production.

Finally, I'd generally expect that DevelopmentBaseConfiguration supports a Docker Compose setup where all other containers are internal supporting services for Django. Django certainly needs to reach other containers, but I can't think of a routine case where other services need to reach Django. Running a headless web browser to print content from Django (I believe the use case that motivated this PR) is an exception, but I'm not sure if it's general enough to support upstream.

So, I'd say that if we can auto-detect the Docker internal network hostname of the container, we should make this change. Otherwise, I think we should close this PR and let interested downstreams just tweak the setting for their needs. If it turns out that lots of users need this feature, I'd be open to reconsidering.

zachmullen commented 3 years ago

Ok, I will fix this downstream.