cookiecutter / cookiecutter-django

Cookiecutter Django is a framework for jumpstarting production-ready Django projects quickly.
https://cookiecutter-django.readthedocs.io
BSD 3-Clause "New" or "Revised" License
12k stars 2.88k forks source link

split settings #3245

Closed ayushin closed 1 month ago

ayushin commented 3 years ago

Description

It would be great to follow django best practices and setup split settings instead of one huge monolith base.py out of the box.

Proposed structure:

sections/base.py
sections/auth.py
sections/security.py
sections/databases.py
sections/templates.py
sections/storages.py

etc

then base.py would only consist of a bunch of

from sections/base.py import env
from sections/base.py import *
from sections/databases.py import *

etc

Rationale

There is no good reason to have one huge unmanageable base.py

Andrew-Chen-Wang commented 3 years ago

would there be a circular dependency error when you try to import env from those separate sections? Unless you're saying import based on file, in which case, I don't think people would be happy to always need to import separate files for each environment they're deploying in.

ayushin commented 3 years ago

ah not really, that's the beauty, as I mentioned above, all configuration is inside sections/* and base.py and production.py and local.py just import those parts.

and if you need some data from another section you just do a clean from .base import env inside of that section so it all works like a clock without any hacks

ayushin commented 3 years ago

you would go further and import all sections inside sections/__init__.py and just have one from sections import * in your production.py or local.py so top level base.py is not needed at all

Andrew-Chen-Wang commented 3 years ago

and if you need some data from another section you just do a clean from .base import env inside of that section so it all works like a clock without any hacks

I'm confused wouldn't that be the circular dependency though (I'm assuming that import is inside a section file?)? Idk I could just be braindead at night right now.

I guess now I'm not understanding you're splitting the sections/** files, or within each file, based on environment. E.g. if we do sections/database.py Do you mind giving me like a small configuration example of a couple files like -- assuming we're in the settings directory: base.py, production.py, local.py, sections/database.py? That'd be really helpful in clarifying this. Thanks!

ayushin commented 3 years ago

The (working) setup I have right now:

settings
├── __init__.py
├── base.py
├── local.py
├── production.py
├── sections
│   ├── __init__.py
│   ├── accounts.py
│   ├── admin.py
│   ├── apps.py
│   ├── base.py
│   ├── celery.py
│   ├── data.py
│   ├── db.py
│   ├── drf.py
│   ├── email.py
│   ├── logging.py
│   ├── middleware.py
│   ├── push_notifications.py
│   ├── security.py
│   ├── storages.py
│   └── templates.py
└── test.py

my settings/base.py:

from .sections.base import *  # noqa
from .sections.apps import *  # noqa
from .sections.accounts import *  # noqa
from .sections.middleware import *  # noqa
from .sections.templates import *  # noqa
from .sections.email import *  # noqa
from .sections.celery import *  # noqa
from .sections.db import *  # noqa
from .sections.admin import *  # noqa
from .sections.drf import *  # noqa
from .sections.security import *  # noqa
from .sections.storages import *  # noqa
from .sections.data import *  # noqa
from .sections.push_notifications import *  # noqa

my local.py:

from .sections.base import *  # noqa
from .sections.base import env, ROOT_DIR

env.read_env(str(ROOT_DIR / ".env"))  # noqa

from .base import *  # noqa

DEBUG = True
.....

my sections/db.py' (could bedatabases.py`):

from .base import env

# DATABASES
# ------------------------------------------------------------------------------
# https://docs.djangoproject.com/en/dev/ref/settings/#databases

DATABASES = {
    "default": env.db("DATABASE_URL", default="postgres:///qta"),
}
DATABASES["default"]["ATOMIC_REQUESTS"] = True

etc

some files have from .base import env, some have from .base import * some don't need any of it

In my opinion this is a little price to pay for clean manageable code and this way I always know which section is using data from another

Andrew-Chen-Wang commented 3 years ago

gotcha thanks for adding this!! I agree this is so much more manageable, but I like the current way our production.py makes sure there are no defaults in production.py. I think if we inherited from Django-environ's Environment class (don't know the name), somehow overrode all their methods to make sure that if we were returned a default with DEBUG=False, then we'd error. That's personal opinion though, and I'm not sure how easy of an implementation that would be...

Note: if the returned value equals the default, then how would we check that? Maybe force setattr() on the default value beforehand to so we can compare if the returned value has the attribute or not? just thinking....

ayushin commented 3 years ago

gotcha thanks for adding this!! I agree this is so much more manageable, but I like the current way our production.py makes sure there are no defaults in production.py. I think if we inherited from Django-environ's Environment class (don't know the name), somehow overrode all their methods to make

This is exactly the same in the new setup of course.

Andrew-Chen-Wang commented 3 years ago

maybe I'm not imagining this correctly, but are you suggesting that there is a database.py and a prod_database.py to be imported separately? In our local.py, we could have env("VAR", default="asdf") whereas in production.py, the variable is override as env("VAR") with no default. So I guess I'm confused where the switch between allowing defaults and not is made.

ayushin commented 3 years ago

No need for separate database and prod_database again:

sections/base.py:

import environ
from pathlib import Path

env = environ.Env(
    DJANGO_LOG_LEVEL=(str, 'INFO'),
    DOMAIN=(str, 'turnaround.aero'),
)

ROOT_DIR = Path(__file__).resolve(strict=True).parent.parent.parent.parent
APPS_DIR = ROOT_DIR / "qta"
DOMAIN = env('DOMAIN')
APP_URL = f'https://app.{DOMAIN}'

USE_I18N = True
USE_L10N = True
USE_TZ = True
TIME_ZONE = "UTC"

CRISPY_TEMPLATE_PACK = "bootstrap4"
DEBUG = env.bool("DJANGO_DEBUG", False)
FIXTURE_DIRS = (str(APPS_DIR / "fixtures"),)
FORM_RENDERER = "django.forms.renderers.TemplatesSetting"
LANGUAGE_CODE = "en-us"
LOCALE_PATHS = [str(ROOT_DIR / "locale")]
MIGRATION_MODULES = {"sites": "qta.contrib.sites.migrations"}
ROOT_URLCONF = "config.urls"
SITE_ID = 1
WSGI_APPLICATION = "config.wsgi.application"

so sections/database.py:

from .base import env

# DATABASES
# ------------------------------------------------------------------------------
# https://docs.djangoproject.com/en/dev/ref/settings/#databases

DATABASES = {
    "default": env.db("DATABASE_URL", default="postgres:///qta"),
}
DATABASES["default"]["ATOMIC_REQUESTS"] = True

It is exactly as before/now.

But yes it does also give you a possibility to hand pick sections for local and production and test if you wanted to.

ayushin commented 3 years ago

The only issue I'm still wondering about is how to make sure I also import if DEBUG: ... kind of settings, cause from X import * seem to ignore those

Actually this works.

Andrew-Chen-Wang commented 3 years ago

hmm I guess I personally prefer the default three settings file, but I'd love to see a new option appear (i.e. an option for sections vs. the three defaults mostly because I use PyCharm and I don't think I'll be able to get any autocomplete anymore)! I think it'd be easy to do using jinja to import other templates if the three defaults were chosen. But yea, would love to see a pr for having both options available!

HHHMHA commented 1 year ago

I was about to suggest the same when I found this issue

Any updates about the status of this issue? Can I work on it as I tested it and it works with pycharm, pytest and docker perfectly.

Here is the structure: image

we import everything and set the environemnt in the upper __init__.py file:

# Components
from .components import *  # noqa
from .components.authentication import *  # noqa
from .components.base import *  # noqa
from .components.crispy import *  # noqa
from .components.databases import *  # noqa
from .components.email import *  # noqa
from .components.installed_apps import *  # noqa
from .components.logging import *  # noqa
from .components.middlewares import *  # noqa
from .components.notifications import *  # noqa
from .components.rest_framework import *  # noqa
from .components.rq import *  # noqa
from .components.static import *  # noqa
from .components.templates import *  # noqa

# Envs
settings_env = env.get_value("ENVIRONMENT", default="local")  # noqa

if settings_env == "local":
    from .environments.local import *  # noqa
elif settings_env == "production":
    from .environments.production import *  # noqa

# Override settings:
try:
    from .environments.override import *  # noqa
except ImportError:
    pass

override is ignored by git too. This really makes settings management easy.

foarsitter commented 1 year ago

@HHHMHA sure, looking forward to your pull-request.

Personally, I prefer searching with tools. I can't remember if the setting is a middleware or belongs to crispy because it is a crispy middleware, so I have to search anyway. Using a file tree for searching is not that fast.

browniebroke commented 1 year ago

I haven't voiced my opinion on this, but I'm also slightly against splitting setting further than how they are split now: I find it harder to search and as just mentioned above, some things might fit in more than 1 place.

HHHMHA commented 1 year ago

I can make the split setting thing optional (will add it to cookiecutter questions and use post_gen_project hook to set them) the downside is everytime settings changes in one place then the other place needs to be updated in the template.

So what do you think

browniebroke commented 1 year ago

My first reaction is that making it optional doesn't seem easy to maintain in the longer run:

  1. As you say, it's duplicating each setting in 2 places.
  2. Testing will become harder as we'll probably have to do it against each other option

We should be opiniated on this: either we do it or not.

PS: Adding nuance to my previous message (I was on my phone), I'm slightly against but that might be because I don't see how it would look like.

luzfcb commented 1 year ago

@browniebroke I agree that it will make maintenance more difficult than it already is. I'm -1 to this change

HHHMHA commented 1 year ago

Adding it will just make settings a lot cleaner. I've seen projects where the settings files are ~2000 Lines. here most components will have few lines of code for example: authentication.py:

# AUTHENTICATION
# ------------------------------------------------------------------------------
# https://docs.djangoproject.com/en/dev/ref/settings/#authentication-backends
AUTHENTICATION_BACKENDS = [
    "django.contrib.auth.backends.ModelBackend",
]
# https://docs.djangoproject.com/en/dev/ref/settings/#auth-user-model
AUTH_USER_MODEL = "users.User"
# https://docs.djangoproject.com/en/dev/ref/settings/#login-redirect-url
LOGIN_REDIRECT_URL = "home"
LOGOUT_REDIRECT_URL = "login"
# https://docs.djangoproject.com/en/dev/ref/settings/#login-url
LOGIN_URL = "login"

# PASSWORDS
# ------------------------------------------------------------------------------
# https://docs.djangoproject.com/en/dev/ref/settings/#password-hashers
PASSWORD_HASHERS = [
    # https://docs.djangoproject.com/en/dev/topics/auth/passwords/#using-argon2-with-django
    "django.contrib.auth.hashers.Argon2PasswordHasher",
    "django.contrib.auth.hashers.PBKDF2PasswordHasher",
    "django.contrib.auth.hashers.PBKDF2SHA1PasswordHasher",
    "django.contrib.auth.hashers.BCryptSHA256PasswordHasher",
]
# https://docs.djangoproject.com/en/dev/ref/settings/#auth-password-validators
AUTH_PASSWORD_VALIDATORS = [
    {"NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator"},
    {"NAME": "django.contrib.auth.password_validation.MinimumLengthValidator"},
    {"NAME": "django.contrib.auth.password_validation.CommonPasswordValidator"},
    {"NAME": "django.contrib.auth.password_validation.NumericPasswordValidator"},
]

crispy.py

CRISPY_ALLOWED_TEMPLATE_PACKS = "bootstrap5"

CRISPY_TEMPLATE_PACK = "bootstrap5"

installed_apps.py

# APPS
# ------------------------------------------------------------------------------
DJANGO_APPS = [
    "django.contrib.auth",
    "django.contrib.contenttypes",
    "django.contrib.sessions",
    "django.contrib.sites",
    "django.contrib.messages",
    "django.contrib.staticfiles",
    "django.contrib.humanize",  # Handy template tags
    "jet",  # not a django app but must come before admin
    "django.contrib.admin",
    "django.forms",
]
THIRD_PARTY_APPS = [
    "rest_framework",
    "rest_framework.authtoken",
    "corsheaders",
    "crispy_forms",
    "crispy_bootstrap5",
    "drf_spectacular",
    "compressor",
    "django_rq",
    "import_export",
]

LOCAL_APPS = [
    "sports_league.common",
    "sports_league.sports",
    "sports_league.users",
    # Your stuff: custom apps go here
]
# https://docs.djangoproject.com/en/dev/ref/settings/#installed-apps
INSTALLED_APPS = DJANGO_APPS + THIRD_PARTY_APPS + LOCAL_APPS
github-actions[bot] commented 1 month ago

As discussed, we won't be implementing this. Automatically closing.