Skip to content

Migrate to django-environ #244

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.

Sign up for GitHub

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

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

Migrate to django-environ #244

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a8c8796
Migrate to django-environ
tkdchen Oct 6, 2019
f376341
Warn email schema console:// to be deprecated
tkdchen Oct 12, 2019
499380b
Merge branch 'master' into migrate-to-django-environ
auvipy Oct 27, 2021
ec51904
Update setup.py
auvipy Oct 27, 2021
3077eaf
Update setup.py
auvipy Oct 27, 2021
fb7417f
Update setup.py
auvipy Oct 27, 2021
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
9 changes: 5 additions & 4 deletions configurations/values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from django.core import validators
from django.core.exceptions import ValidationError, ImproperlyConfigured
from django.utils import six
from environ import Env

from .utils import import_by_path, getargspec

Expand Down Expand Up @@ -414,7 +415,7 @@ def setup(self, name):


class EmailURLValue(CastingMixin, MultipleMixin, Value):
caster = 'dj_email_url.parse'
caster = Env.email_url_config
message = 'Cannot interpret email URL value {0!r}'
late_binding = True

Expand Down Expand Up @@ -449,21 +450,21 @@ def to_python(self, value):


class DatabaseURLValue(DictBackendMixin, CastingMixin, Value):
caster = 'dj_database_url.parse'
caster = Env.db_url_config
message = 'Cannot interpret database URL value {0!r}'
environ_name = 'DATABASE_URL'
late_binding = True


class CacheURLValue(DictBackendMixin, CastingMixin, Value):
caster = 'django_cache_url.parse'
caster = Env.cache_url_config
message = 'Cannot interpret cache URL value {0!r}'
environ_name = 'CACHE_URL'
late_binding = True


class SearchURLValue(DictBackendMixin, CastingMixin, Value):
caster = 'dj_search_url.parse'
caster = Env.search_url_config
message = 'Cannot interpret Search URL value {0!r}'
environ_name = 'SEARCH_URL'
late_binding = True
9 changes: 1 addition & 8 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,11 @@ def find_version(*parts):
'django-cadmin = configurations.management:execute_from_command_line',
],
},
install_requires=['django-environ'],
extras_require={
'cache': ['django-cache-url'],
'database': ['dj-database-url'],
'email': ['dj-email-url'],
'search': ['dj-search-url'],
'testing': [
'django-discover-runner',
'mock',
'django-cache-url>=1.0.0',
'dj-database-url',
'dj-email-url',
'dj-search-url',
'six',
'Sphinx>=1.4',
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Could maybe still be in extras_require?

Copy link
Author

Choose a reason for hiding this comment

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

Looks no need here. How useful to keep those in extra_require?

Copy link
Contributor

Choose a reason for hiding this comment

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

It avoids having to install / installing it by default when not needed.

I guess the typical usecase is using URLs for configs then, so that seems to be OK.

Expand Down
19 changes: 4 additions & 15 deletions tests/test_values.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,6 @@ def test_database_url_value(self):
with env(DATABASE_URL='sqlite://'):
self.assertEqual(value.setup('DATABASE_URL'), {
'default': {
'CONN_MAX_AGE': 0,
'ENGINE': 'django.db.backends.sqlite3',
'HOST': '',
'NAME': ':memory:',
Expand Down Expand Up @@ -411,26 +410,23 @@ def test_email_url_value(self):
'EMAIL_HOST_PASSWORD': 'password',
'EMAIL_HOST_USER': '[email protected]',
'EMAIL_PORT': 587,
'EMAIL_USE_SSL': False,
'EMAIL_USE_TLS': True})
with env(EMAIL_URL='console://'):
with env(EMAIL_URL='consolemail://'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that change required?
Is it related to https://github.com/psgs/ConsoleMail?

Copy link
Author

Choose a reason for hiding this comment

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

Is it related to https://github.com/psgs/ConsoleMail?

No. I'm not aware of ConsoleMail. What I learned from django-environ is only consolemail is defined in email schemes, but no console. The consolemail maps to same backend django.core.mail.backends.console.EmailBackend.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's another behavior change then apparently AFAIK.

All those might be fine, and it's good to modernize, but might break setups/configs.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @blueyed, I added another commit to make console:// still work in apps using it, and deprecate console://.

self.assertEqual(value.setup('EMAIL_URL'), {
'EMAIL_BACKEND': 'django.core.mail.backends.console.EmailBackend', # noqa: E501
'EMAIL_FILE_PATH': '',
'EMAIL_HOST': None,
'EMAIL_HOST_PASSWORD': None,
'EMAIL_HOST_USER': None,
'EMAIL_PORT': None,
'EMAIL_USE_SSL': False,
'EMAIL_USE_TLS': False})
'EMAIL_PORT': None})
with env(EMAIL_URL='smtps://[email protected]:[email protected]:wrong'): # noqa: E501
self.assertRaises(ValueError, value.setup, 'TEST')

def test_cache_url_value(self):
cache_setting = {
'default': {
'BACKEND': 'django_redis.cache.RedisCache',
'LOCATION': 'redis://host:6379/1',
'LOCATION': 'redis://user@host:6379/1',
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently it changes some defaults (more secure by default).

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if I get your point. Is this change acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It changes defaults apparently, so we should bump the major or at least minor version for it.

}
}
cache_url = 'redis://user@host:6379/1'
Expand All @@ -443,13 +439,7 @@ def test_cache_url_value(self):
with env(CACHE_URL='wrong://user@host:port/1'):
with self.assertRaises(Exception) as cm:
value.setup('TEST')
self.assertEqual(cm.exception.args[0], 'Unknown backend: "wrong"')
with env(CACHE_URL='redis://user@host:port/1'):
with self.assertRaises(ValueError) as cm:
value.setup('TEST')
self.assertEqual(
cm.exception.args[0],
"Cannot interpret cache URL value 'redis://user@host:port/1'")
self.assertEqual(cm.exception.args[0], 'wrong')
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks "wrong"?!
Shouldn't it have more info / a full msg?

Copy link
Author

Choose a reason for hiding this comment

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

django-environ simply raises KeyError when looking up a cache scheme by wrong, and here assertRaises catches the KeyError.

It might be good for django-envion to catch the KeyError and reraise a specific error with more descriptive message.

What do you think?

Copy link
Author

@tkdchen tkdchen Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

FYI, I made a patch to django-environ, joke2k/django-environ#235

After this patch is accepted and merged, this assertion could be updated accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

@tkdchen , that patch was merged; I guess this can be updated now ?


def test_search_url_value(self):
value = SearchURLValue()
Expand Down Expand Up @@ -503,7 +493,6 @@ class Target(object):
'EMAIL_HOST_PASSWORD': 'password',
'EMAIL_HOST_USER': '[email protected]',
'EMAIL_PORT': 587,
'EMAIL_USE_SSL': False,
'EMAIL_USE_TLS': True
})
self.assertEqual(
Expand Down