Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Flexibility when establishing database connection #12

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

Flexibility when establishing database connection #12

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d93dde8
support establishing DB connection with MAX_CONN_AGE (for django chan…
burcaw Mar 26, 2018
5cff962
fixed ssl_required reference before assignment
burcaw Mar 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions django_heroku/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=1
  • USE_MAX_CONN_AGE=0
  • USE_MAX_CONN_AGE=True
  • USE_MAX_CONN_AGE=False
  • USE_MAX_CONN_AGE=false
  • USE_MAX_CONN_AGE=fALSE
  • USE_MAX_CONN_AGE=not-a-bool

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 = 600

tbh, 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.

else:
use_max_conn_age = True

# DEVELOPMENT mode configuration.
ssl_required = True
if 'DEVELOPMENT' in os.environ:
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':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, many people like using 1 and 0 for boolean values. This is very limiting

ssl_required = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_AGE

If someone wants to use it and then here we set

dj_database_config['ssl_required'] = True  # or False

And 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)

Copy link

@kcolton kcolton Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sigmavirus24 @burcaw

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".


# Database configuration.
# TODO: support other database (e.g. TEAL, AMBER, etc, automatically.)
Expand All @@ -60,13 +73,19 @@ def settings(config, *, db_colors=False, databases=True, test_runner=True, stati

logger.info('Adding ${} to DATABASES Django setting ({}).'.format(env, db_color))

config['DATABASES'][db_color] = dj_database_url.parse(url, conn_max_age=MAX_CONN_AGE, ssl_require=True)
if use_max_conn_age:
config['DATABASES'][db_color] = dj_database_url.parse(url, conn_max_age=MAX_CONN_AGE, ssl_require=True)
else:
config['DATABASES'][db_color] = dj_database_url.parse(url, ssl_require=True)

if 'DATABASE_URL' in os.environ:
logger.info('Adding $DATABASE_URL to default DATABASE Django setting.')

# Configure Django for DATABASE_URL environment variable.
config['DATABASES']['default'] = dj_database_url.config(conn_max_age=MAX_CONN_AGE, ssl_require=True)
if use_max_conn_age:
config['DATABASES']['default'] = dj_database_url.config(conn_max_age=MAX_CONN_AGE, ssl_require=ssl_required)
else:
config['DATABASES']['default'] = dj_database_url.config(ssl_require=ssl_required)

logger.info('Adding $DATABASE_URL to TEST default DATABASE Django setting.')

Expand Down