Flexibility when establishing database connection #12
Conversation
…nels 2) and not requiring SSL when in development mode
| @@ -43,6 +43,19 @@ def teardown_databases(self, old_config, **kwargs): | |||
|
|
|||
|
|
|||
| def settings(config, *, db_colors=False, databases=True, test_runner=True, staticfiles=True, allowed_hosts=True, logging=True, secret_key=True): | |||
| if 'USE_MAX_CONN_AGE' in os.environ: | |||
| use_max_conn_age = bool(os.environ['USE_MAX_CONN_AGE']) | |||
There was a problem hiding this comment.
Most people want 1 to mean True and 0 to mean False but this will always evaluate to True regardless of the following values:
USE_MAX_CONN_AGE=1USE_MAX_CONN_AGE=0USE_MAX_CONN_AGE=TrueUSE_MAX_CONN_AGE=FalseUSE_MAX_CONN_AGE=falseUSE_MAX_CONN_AGE=fALSEUSE_MAX_CONN_AGE=not-a-bool
There was a problem hiding this comment.
@sigmavirus24 @burcaw What if the line was:
try:
# already defined in settings before
MAX_CONN_AGE
except NameError:
# assuming no preference. set a sensible default
# 600 doesn't impact local and makes heroku's postgres servers happier
MAX_CONN_AGE = 600tbh, this setting I used to update in my setting_prod.py with DATABASES['defaut']['CONN_MAX_AGE'] = 600. django's default of 0 is rather silly and adds extra connection overhead without people knowing. For most indivudals not a big deal, but across an entire infrastructure I can see why these opionated defaults are in place.
| logger.info('Adding $DEVELOPMENT to DEVELOPMENT Django setting.') | ||
| # Set the Django setting from the environment variable. | ||
| config['DEVELOPMENT'] = os.environ['DEVELOPMENT'] | ||
| if os.environ['DEVELOPMENT'] == 'True': |
There was a problem hiding this comment.
Again, many people like using 1 and 0 for boolean values. This is very limiting
| # Set the Django setting from the environment variable. | ||
| config['DEVELOPMENT'] = os.environ['DEVELOPMENT'] | ||
| if os.environ['DEVELOPMENT'] == 'True': | ||
| ssl_required = False |
There was a problem hiding this comment.
What if we have a dictionary, perhaps named dj_database_config that aggregates these options. So above we would set
dj_database_config['conn_max_age'] = MAX_CONN_AGEIf someone wants to use it and then here we set
dj_database_config['ssl_required'] = True # or FalseAnd then below where we .parse() or .config() we just have one call, e.g.,
config['DATABASES'][db_color] = dj_database_url.parse(url, **dj_database_config)and
config['DATABASES']['default'] = dj_database_url.config(**dj_database_config)There was a problem hiding this comment.
What if this was simply:
ssl_require = not DEBUG
I agree this particular setting is counter intuitive and tripped me up.
What if we have a dictionary, perhaps named dj_database_config that aggregates these options. So above we would set
More specific settings could still be tweaked by updating DATABASES['default'] dict after django_heroku does its thing, so there is already a way to tweak settings. This one, however, would be nice to "just work".
Here is a pass at providing flexibility in establishing the database connection per the following two issues:
Permit disabling 'require_ssl' when running locally for development
#10
Ability to Override MAX_CONN_AGE
#11
I welcome any feedback to make these changes acceptable for inclusion.