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

DATABASES setting for Django not being set #89

Closed
dkliban opened this issue Sep 28, 2018 · 13 comments
Closed

DATABASES setting for Django not being set #89

dkliban opened this issue Sep 28, 2018 · 13 comments

Comments

@dkliban
Copy link

dkliban commented Sep 28, 2018

dynaconf is properly showing the settings I put into /etc/pulp/settings.py

(pulp) [vagrant@pulp3 pulp]$ dynaconf list -k DATABASES
Django app detected
Working in development environment 
DATABASES: {'default': {'CONN_MAX_AGE': 0,
             'ENGINE': 'django.db.backends.dummy',
             'HOST': 'postgres',
             'NAME': 'pulp',
             'PORT': '5432',
             'USER': 'pulp'}}

However, django is not using the setting. It seems like Django is loading the settings for the database before dynaconf is loaded.

@rochacbruno
Copy link
Member

rochacbruno commented Sep 28, 2018

Looks like Django is accessing DATABASES before the load of INSTALLED_APPS so I can think in a way to fix it.

Load dynaconf explicitly in manage.py and in wsgi.py

after this line.
https://github.com/rochacbruno/dynaconf/blob/master/example/django_example/manage.py#L6

or calling it on settings.py bottom.

@rochacbruno
Copy link
Member

I want to add an implementation targetting Python 3.7 using this https://www.python.org/dev/peps/pep-0562/

and also I am going to try other solutions.

If we figureout that the proper way is calling dynaconf on manage.py and wsgi.py then I'll add this to the docs.

@dkliban
Copy link
Author

dkliban commented Sep 28, 2018

Thank you! I was able to follow your advice here[0].

[0] https://github.com/pulp/pulp/pull/3678/files

dkliban added a commit to dkliban/pulp that referenced this issue Sep 29, 2018
Solution: load dynaconf in manage.py, wsgi.py, worker.py, and entry_points.py

This patch introduces a workaround for a dynaconf issue[0].

This patch also changes how Pulp is deployed on Travis. A non-default name is used for the database. That
way we will know if the settings in the settings file are being used.

[0] dynaconf/dynaconf#89

closes: #4046
https://pulp.plan.io/issues/4046
dkliban added a commit to dkliban/pulp that referenced this issue Sep 29, 2018
Solution: load dynaconf in manage.py, wsgi.py, worker.py, and entry_points.py

This patch introduces a workaround for a dynaconf issue[0].

This patch also changes how Pulp is deployed on Travis. A non-default name is used for the database. That
way we will know if the settings in the settings file are being used.

[0] dynaconf/dynaconf#89

closes: #4046
https://pulp.plan.io/issues/4046
@rochacbruno
Copy link
Member

rochacbruno commented Sep 30, 2018

I did an extensive dive in to Django source code today,

The case is that Django is creating the database connection before it loads the INSTALLED_APPS where dynaconf is supposed to be loaded.

Django is doing that because there is a console hint saying You have N pending migrations to be applied... and to calculate that Django fires the migration check before the INSTALLED_APPS are loaded.

This is part of https://docs.djangoproject.com/en/1.11/ref/checks/#database

The only setting that Django is looking before INSTALLED_APPS is DATABASES

One of the possible solution is introducing a new environment variable for Django so users will always need to export the following 2 variables to enable Dynaconf:

export DJANGO_SETTINGS_MODULE=dynaconf.contrib.django_settings
export SETTINGS_MODULE_FOR_DYNACONF=myprogram.app.settings

Then internally the dynaconf.contrib.django_settings resolves the load of myprogram.app.settings the solution works seemlesly without the need to even change the INSTALLED APPS, the good part is that to enable dynaconf the user only needs to change envvars and no need to touch any code.

I want to hear more opinions about this idea ^.

For now I will:

  • Keep this issue opened until we find a better solution (Still need to try the Django AppConfig class)
  • Send an email do Django dev list asking for help/suggestions
  • Include in the Dynaconf docs a note saying that:
    • "If you need the DATABASES to be rewritable please load dynaconf in manage.py, wsgi.py and any other entry point of your application intead of INSTALLED_APPS use INSTALLED_APPS only in cases where you don't need the DATABASES key to be managed by dynaconf."
  • For Python 3.7 I'll include an implementation using the new module.__getattr__ method

IN all the cases, the backwards compatibility should never break! so the way you are loading dynaconf today should still work and the way using INSTALLED_APPS should also work. My plan is to introduce other ways to load it.

@mspinelli
Copy link
Contributor

mspinelli commented Nov 14, 2018

Any progress on this problem?

I want to hear more opinions about this idea ^.

I haven't tried the dual environment variable export yet. Are you saying this works out of the box already this way, or that it would need more code changes in dynaconf to make this work? E.g. dynaconf.contrib.django_settings doesn't seem to exist.

That said I am on Python 3.7. What does the module.__getattr__ method look like to solve this problem?

@rochacbruno
Copy link
Member

@mspinelli the proposed solution is not implemented yet, it needs to be implemented.

Right now to use django there are 2 solutions

    1. Load dynaconf in INSTALLED_APPS knowing that DATABASE should be kept on django's settings.
    1. Load dynaconf in all the entry points as manage.py, wsgi.py and others.

openstack-gerrit pushed a commit to recordsansible/ara-server that referenced this issue Dec 20, 2018
This changes the configuration engine from everett to dynaconf.
dynaconf allows loading configuration from files (json, ini, yaml, toml)
as well as environment variables prefixed by ARA_.

Our usage of dynaconf is similar to the use case from the Pulp [1]
project and they have documented an issue when loading database
parameters [2]. This issue is worked around by importing dynaconf in the
different entry points.

This introduces some other changes as well:
- We're now creating a default configuration and data directory at
  ~/.ara. The location of this directory is controlled with the
  ARA_BASE_DIR environment variable.
- We're now creating a default configuration template in
  ~/.ara/default_config.yaml.
- The default database is now located at ~/.ara/ara.sqlite. The location
  of this database can be customized with the ARA_DATABASE_NAME
  environment variable.
  Note that ARA 0.x used "~/.ara/ansible.sqlite" -- the file name change
  is deliberate in order to avoid user databases clashing between
  versions.

More documentation on this will be available in an upcoming patch.

[1]: https://github.com/pulp/pulp
[2]: dynaconf/dynaconf#89

Change-Id: I8178b4ca9f2b4d7f4c45c296c08391e84e8b990d
@dmsimard
Copy link
Contributor

Hi o/

By the way, I think @apollo13 had a good idea by getting dynaconf to load settings from within settings instead of patching entrypoints for this issue.

You can see the commit here: recordsansible/ara-server@fdcc003

@rochacbruno
Copy link
Member

@dmsimard @apollo13 can we expect a Pull Request?

@apollo13
Copy link
Contributor

@rochacbruno Against what? My changes just use dynaconf directly and drop the Django integration. There is not really anything to patch with that regard in dynaconf itself…

@jasisz
Copy link

jasisz commented Feb 19, 2019

I've found two more Django corner-cases.
One is ALLOWED_HOSTS - even if you change it with dynaconf, it still will have default value somewhere internally and requests will fail.
The other case is when you use settings.SOME_CONFIG in your templates - in such scenario dynaconf fails with 'Settings' object has no attribute '__name__' - this is because it goes through the django.templates.base._resolve_lookup which inspects with getcallargs on LazySettings.

@rochacbruno
Copy link
Member

The template problem must e resolved with the recommendation to use ['key]` syntax.

The other problems there is a way to fix using getattr but only for python 3.7+

@rochacbruno
Copy link
Member

IN progress on #126

@rochacbruno rochacbruno modified the milestones: 1.2.x, 1.3.0 Mar 28, 2019
@rochacbruno
Copy link
Member

2.0.0 released DATABASE stuff is now fixed.
read more in https://dynaconf.readthedocs.io/en/latest/guides/django.html
example in https://github.com/rochacbruno/dynaconf/tree/master/example/django_example

@jasisz can you please test the render template? or add a reproducer on the https://github.com/rochacbruno/dynaconf/tree/master/example/django_example

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants