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

Fixed problem with env in settings.py and updated README about ALLOWED_HOSTS #42

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

Conversation

fklemme
Copy link
Contributor

@fklemme fklemme commented Sep 29, 2021

A couple of improvements from user feedback on discord.
Apparently, environment variables coming from env are string by default and not automatically cast to higher-level types such as lists. To ensure correct behavior, I rewrote the env lines in settings.py to only depend on strings.
I also added a small note on DJANGO_ALLOWED_HOSTS to the README as it caused some trouble in a setup.

@ndepaola
Copy link
Collaborator

i thought django-environ handled type casting of environment variables? https://django-environ.readthedocs.io/en/latest/

@fklemme
Copy link
Contributor Author

fklemme commented Sep 29, 2021

I saw examples where the user configures explicit casting. But I think that will work only to some extend. Ofc, python will be able to handle something like int("2"). But I would not trust what happens to list("['a', 'b']"). But I'm also not a python expert, so I'm happy to be proven wrong.

@fklemme
Copy link
Contributor Author

fklemme commented Sep 29, 2021

Okay, I see there is some "smart casting'. Lists need to be handled differently. Let me have another try on this one...

@fklemme
Copy link
Contributor Author

fklemme commented Sep 29, 2021

Hmm, so after having a deeper look at smart casting and default values, I'm not even sure anymore if this is something we want to deal with. There are a lot of side effects and pit holes discussed on the django-environ github page. Apparently, these problems became even so dominant that smart scaling will be disabled in upcoming releases by default [1].

I agree that the code in this PR is not looking as smooth as the previous solution, but seeing the possible misinterpretations, I would consider it solving it as proposed for now.

Edit: Maybe one last note, because it might not have been clear in the description: The current implementation of ALLOWED_HOSTS is not working / cannot be changed through the .env file. So this implementation fixes that problem.

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