Migrate to django-environ #244

tkdchen · 2019-10-06T14:31:57Z

This patch uses django-environ and drop deprecated and old packages
django-cache-url, dj-email-url, dj-database-url and dj-cache-url.

Tests are updated according to the behavior of django-environ.

Signed-off-by: Chenxiong Qi [email protected]

This patch uses django-environ and drop deprecated and old packages django-cache-url, dj-email-url, dj-database-url and dj-cache-url. Tests are updated according to the behavior of django-environ. Signed-off-by: Chenxiong Qi <[email protected]>

codecov · 2019-10-06T14:38:07Z

Codecov Report

Merging #244 (f376341) into master (089a039) will decrease coverage by 0.32%.
The diff coverage is 100.00%.

❗ Current head f376341 differs from pull request most recent head fb7417f. Consider uploading reports for the commit fb7417f to get more accurate results

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
- Coverage   90.08%   89.75%   -0.33%     
==========================================
  Files          27       25       -2     
  Lines        1210     1201       -9     
  Branches      101      107       +6     
==========================================
- Hits         1090     1078      -12     
- Misses         89       90       +1     
- Partials       31       33       +2

Flag	Coverage Δ
coverage	`89.67% <100.00%> (?)`
dj111	`89.67% <100.00%> (?)`
dj20	`88.59% <100.00%> (?)`
dj21	`88.42% <100.00%> (?)`
py27	`89.09% <100.00%> (?)`
py34	`88.59% <100.00%> (?)`
py35	`88.59% <100.00%> (?)`
py36	`88.59% <100.00%> (?)`
py37	`88.59% <100.00%> (?)`
pypy	`89.09% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
configurations/values.py	`95.22% <100.00%> (+0.90%)`	⬆️
tests/test_values.py	`100.00% <100.00%> (ø)`
tests/settings/main.py	`88.23% <0.00%> (-1.51%)`	⬇️
configurations/base.py	`82.45% <0.00%> (-0.88%)`	⬇️
tests/test_main.py	`95.23% <0.00%> (-0.51%)`	⬇️
tests/urls.py	`100.00% <0.00%> (ø)`
tests/test_env.py	`100.00% <0.00%> (ø)`
tests/docs/conf.py	`100.00% <0.00%> (ø)`
tests/setup_test.py	`100.00% <0.00%> (ø)`
tests/test_sphinx.py	`100.00% <0.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 089a039...fb7417f. Read the comment docs.

blueyed · 2019-10-08T23:44:25Z

tests/test_values.py

-            self.assertEqual(
-                cm.exception.args[0],
-                "Cannot interpret cache URL value 'redis://user@host:port/1'")
+            self.assertEqual(cm.exception.args[0], 'wrong')


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

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?

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.

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

blueyed · 2019-10-08T23:45:08Z

tests/test_values.py

        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',


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

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

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

blueyed · 2019-10-08T23:47:24Z

tests/test_values.py

                'EMAIL_USE_TLS': True})
-        with env(EMAIL_URL='console://'):
+        with env(EMAIL_URL='consolemail://'):


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

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.

That's another behavior change then apparently AFAIK.

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

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

blueyed · 2019-10-08T23:48:15Z

setup.py

-            'django-cache-url>=1.0.0',
-            'dj-database-url',
-            'dj-email-url',
-            'dj-search-url',
            'six',
            'Sphinx>=1.4',
        ],


Could maybe still be in extras_require?

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

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.

Apps using schema console:// still works, which is converted to consolemail:// internally in order to work with django-environ. Meanwhile, a deprecation warning is output for console://. Signed-off-by: Chenxiong Qi <[email protected]>

pauloxnet · 2019-11-06T14:25:24Z

I think django-environ is a strong addiction. I would prefer to leave django-configuration as light as possible.

danpalmer · 2020-06-24T09:45:10Z

Hi team / @tkdchen any update on this? It looks like all code review has been addressed. Would be great to get this merged!

@pauloxnet What's your reasoning for having django-configurations "as light as possible"? I think this is a positive change because:

it unifies more of the Python ecosystem on to django-environ as the parser for configuration URLs
this means more consistency and interoperability with other packages or in other environments
it removes 4 dependencies that need to be managed/bumped/etc, so in a way is "lighter"

carltongibson · 2020-06-24T10:05:42Z

Hi folks.

Can you see also django/django#13029 and the related mailing list discussion?

tl;dr: Maybe we pull something into Django here. Perhaps it makes sense to have env vars read by the default project templates, but we should get right exactly what that should look like, and I don't want us to reinvent something that's already solved™ by the community.

It would be good to get your thoughts as contributors here.

Thanks 😉
C.

auvipy

I am very positive about this change moves o django-environ. can we just rebase and get this merged

brianhelba · 2021-10-27T04:13:50Z

@auvipy It looks like @pauloxnet (who is now helping to maintain this project) objects to this change:

I think django-environ is a strong addiction. I would prefer to leave django-configuration as light as possible.

Could we get a consensus before merging this? If you (@auvipy) are planning to merge things yourself, could you please join the maintainer team and communicate your intentions with the rest of us: #295 (comment)

@pauloxnet Do you still object to this? It looks like django-environ is actively being maintained, whereas it looks like at least dj-database-url is unmaintained and in some sort of strange fork state. In particular, can you respond to @danpalmer's comment:

@pauloxnet What's your reasoning for having django-configurations "as light as possible"? I think this is a positive change because:

it unifies more of the Python ecosystem on to django-environ as the parser for configuration URLs

this means more consistency and interoperability with other packages or in other environments

it removes 4 dependencies that need to be managed/bumped/etc, so in a way is "lighter"

brianhelba

@auvipy There are a some issues with your recent merge. See inline comments.

setup.py

brianhelba · 2021-10-27T04:24:55Z

setup.py

        'testing': [
+            'django-discover-runner',
+            'mock',
            'django-cache-url>=1.0.0',
            'dj-database-url',
            'dj-email-url',


These 4 packages need to be removed from the testing extra too.

brianhelba

The docs for these Value types need to be updated too, as they contain references to the old upstream packages: https://github.com/jazzband/django-configurations/blob/master/docs/values.rst#url-based-values

Users looking for precise syntax info should be pointed to django-environ docs instead.

pauloxnet · 2021-10-27T08:11:29Z

@pauloxnet Do you still object to this? It looks like django-environ is actively being maintained, whereas it looks like at least dj-database-url is unmaintained and in some sort of strange fork state. In particular, can you respond to @danpalmer's comment

I'm going to review again this PR but the main guidelines for me is similar to the one used by Django to strongly avoid to depend on other packages, or depend only on trusted, maintained and really necessary ones.

Co-authored-by: Brian Helba <[email protected]>

auvipy · 2021-10-27T08:16:53Z

@auvipy It looks like @pauloxnet (who is now helping to maintain this project) objects to this change:

I think django-environ is a strong addiction. I would prefer to leave django-configuration as light as possible.

Could we get a consensus before merging this? If you (@auvipy) are planning to merge things yourself, could you please join the maintainer team and communicate your intentions with the rest of us: #295 (comment)

@pauloxnet Do you still object to this? It looks like django-environ is actively being maintained, whereas it looks like at least dj-database-url is unmaintained and in some sort of strange fork state. In particular, can you respond to @danpalmer's comment:

@pauloxnet What's your reasoning for having django-configurations "as light as possible"? I think this is a positive change because:

it unifies more of the Python ecosystem on to django-environ as the parser for configuration URLs

this means more consistency and interoperability with other packages or in other environments

it removes 4 dependencies that need to be managed/bumped/etc, so in a way is "lighter"

I just shared my suggestion. but i strongly believe moving to django-environ would a good move. even django core tried that way.

auvipy · 2021-10-27T08:23:06Z

Also in my humble opinion @pauloxnet reasoning on blocking this is not valid. even if he is maintaining this project he do not have enough contribution history to the project. I am also interested to join the team as well. no offense just sharing my honest thoughts.

pauloxnet · 2021-10-27T10:01:55Z

Also in my humble opinion @pauloxnet reasoning on blocking this is not valid. even if he is maintaining this project he do not have enough contribution history to the project. I am also interested to join the team as well. no offense just sharing my honest thoughts.

No offense at all, you're totally right, but at the same time I'm using this package in a lot of projects as many other users I think that want their projects continue running.

This is PR is a huge changes for this project, it add a totally new dependency and it's not retro compatible.

If you really think start using django-environ is a good move for the project then we have to define a strict goal checks to verify that ti move will not brake project using it and replace all current features (e.g. I see some tests has been removed and it's seems because django-environ can't pass them).

However I think it's a better idea to release the 2.3 version.

auvipy · 2021-10-27T10:18:44Z

it would be better in a v3.0 release

uhurusurfa · 2024-11-11T23:39:45Z

I would agree with @pauloxnet on this one and would go a step further by creating an internal URL checker and converter to remove the dependencies on the 3 external packages. I had to fix a unit test that started failing due to a change in dj_database_url
I would propose closing this PR unless there remains strong opinions to move forward with the django-environ use.

Migrate to django-environ

a8c8796

This patch uses django-environ and drop deprecated and old packages django-cache-url, dj-email-url, dj-database-url and dj-cache-url. Tests are updated according to the behavior of django-environ. Signed-off-by: Chenxiong Qi <[email protected]>

blueyed reviewed Oct 8, 2019

View reviewed changes

Warn email schema console:// to be deprecated

f376341

Apps using schema console:// still works, which is converted to consolemail:// internally in order to work with django-environ. Meanwhile, a deprecation warning is output for console://. Signed-off-by: Chenxiong Qi <[email protected]>

tkdchen force-pushed the migrate-to-django-environ branch from e04768c to f376341 Compare October 12, 2019 03:47

carltongibson mentioned this pull request Jun 24, 2020

Add helpers to get settings values from env variables django/django#13029

Closed

auvipy suggested changes Oct 27, 2021

View reviewed changes

Merge branch 'master' into migrate-to-django-environ

499380b

brianhelba requested changes Oct 27, 2021

View reviewed changes

brianhelba mentioned this pull request Oct 27, 2021

Release version 2.3 on pypi #277

Closed

8 tasks

Asif Saif Uddin and others added 3 commits October 27, 2021 14:13

Update setup.py

ec51904

Co-authored-by: Brian Helba <[email protected]>

Update setup.py

3077eaf

Co-authored-by: Brian Helba <[email protected]>

Update setup.py

fb7417f

Co-authored-by: Brian Helba <[email protected]>

Uh oh!

Migrate to django-environ #244

Are you sure you want to change the base?

Migrate to django-environ #244

Uh oh!

Conversation

tkdchen commented Oct 6, 2019

Uh oh!

codecov bot commented Oct 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkdchen Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloxnet commented Nov 6, 2019

Uh oh!

danpalmer commented Jun 24, 2020

Uh oh!

carltongibson commented Jun 24, 2020

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

brianhelba commented Oct 27, 2021

Uh oh!

brianhelba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianhelba left a comment

Choose a reason for hiding this comment

Uh oh!

pauloxnet commented Oct 27, 2021

Uh oh!

auvipy commented Oct 27, 2021

Uh oh!

auvipy commented Oct 27, 2021

Uh oh!

pauloxnet commented Oct 27, 2021

Uh oh!

auvipy commented Oct 27, 2021

Uh oh!

uhurusurfa commented Nov 11, 2024

Uh oh!

Uh oh!

codecov bot commented Oct 6, 2019 •

edited

Loading

tkdchen Oct 11, 2019 •

edited

Loading