[PULP-210] Update deprecated django storage config #6058

pedro-psb · 2024-11-20T17:46:54Z

Summary

The setting DEFAULT_FILE_STORAGE / STATCIFILES_STORAGE was deprecated in django 4.2. They've implemented a compatibility layer to enable this legacy setting to be mapped to the new STORAGES, so users could gradually move and Pulp codebase to update to using only STORAGES.

The problem is that Pulp overrides the default (using the legacy setting name), so users can't really use STORAGE, as it raises a django exceptions on mutual exclusion.

This PR does:

Updates the codebase to rely on settings.STORAGES instead of DEFAULT_FILE_STORAGE
Enable users to start using STORAGES on their settings
Use both legacy and new setting in CI (s3 / azure) for sanity testing

Open questions:

Removal deadline: I'm suggesting that we remove it on 3.85 or 4.0, but we could choose something else. Deciding this is important so we can give a proper heads up for our users and stakeholders.

Plugins TODO

Check whether the signal handling (on app/apps.py) uses a module-level settings. That could cause errors, so it needs to be moved inside the AppConfig class
Use STORAGES internally instead of DEFAULT_FILE_STORAGE and STATICFILES_STORAGE

Closes #5404

mdellweg · 2024-12-18T13:16:25Z

pulpcore/app/settings.py

+storages._backends = settings.STORAGES.copy()
+storages.backends


Is this the "magic" to prevent django storages to see both items we set before?

No, Django in fact has both set in their default global_settings.
But the previous hack caused a sife-effect on a specific point of django startup where it tries to get those values with a method that can't see the patch we made. The storages is a cached property, so it would hold the wrong value forever.

Line 518 does exactly what they do here.
Line 519 triggers the value to be cached.

All this magic is to give users a larger time span to upgrade.

mdellweg · 2024-12-18T13:17:37Z

pulpcore/app/settings.py

+setattr(global_settings, "DEFAULT_FILE_STORAGE", _DEFAULT_FILE_STORAGE)
+setattr(global_settings, "STORAGES", _STORAGES)


Setting them both here seems to contradict that they are, as you say, "mutually exclusive".

The "global setting" settings is the only place where django won't complain about exclusivity. It will perform this check on a different context.

Sounds weird, but I'll take your word that it works.

mdellweg · 2024-12-18T13:19:46Z

CHANGES/plugin_api/5404.removal

@@ -0,0 +1 @@
+Removed `DEFAULT_FILE_STORAGE` setting in favour of the new `STORAGES` options introduced in Django 4.2.


If I understand, this is a hard breaking change for plugins with storage specific code, right?
Or can plugins still "see" the old setting alongside the new one?

Also we want users to migrate to the new setting (sooner than later). You should add a second changelog for that.

This message is not very accurate, forget to update it.
What I'm really doing is (1) updating codebase to use STORAGES + (2) allowing users to use either of both. I guess it can be two entries, with the second one stressing that now users should start migrating.

Django copies the value of DEFAULT_FILE_STORAGE to STORAGES, but not the other way around.
So yes, plugins should use STORAGES internally (I overlooked that fact).

mdellweg

Just nitpicking on the changelogs...

mdellweg · 2024-12-18T15:27:47Z

CHANGES/plugin_api/5404.removal

+Started using `STORAGES` internally instead of `DEFAULT_FILE_STORAGE` and `STATICFILES_STORAGE`,
+which was deprecated in Django 4.2.
+
+For compatibility, plugins should replace access to:


Suggested change

For compatibility, plugins should replace access to:

For compatibility, plugins must replace access to:

Use strong words where appropriate...

mdellweg · 2024-12-18T15:30:32Z

CHANGES/5404.feature

+These legacy settings were deprecated in Django 4.2 and will be removed in Pulp 3.85 or Pulp 4,
+whichever comes first.


Suggested change

These legacy settings were deprecated in Django 4.2 and will be removed in Pulp 3.85 or Pulp 4,

whichever comes first.

These legacy settings were deprecated in Django 4.2 and will be removed in Pulp 3.85.

I appreciate the effort to get the pulp 4 idea out, but i think that part is implicit and my drag the attention away from what is important here.
Or put another way. I think we can at any time in the future claim that pulp 3.(70+n*15) will actually be pulp 4 and it will inherit the deprecations naturally.

CHANGES/plugin_api/5404.removal

mdellweg · 2025-01-02T10:15:39Z

pulp_file/tests/functional/api/test_download_policies.py

@@ -11,7 +11,6 @@

 from pulpcore.tests.functional.utils import get_files_in_manifest, download_file

-from pulpcore.app import settings


Oh dear, this test was mixing both "imports"...?

Yes, it was

mdellweg · 2025-01-02T10:18:05Z

pulpcore/app/apps.py

+        # Workaround for getting the up-to-date settings instance.
+        # A module-level settings import is a cached version from before dynaconf do its work.
+        from django.conf import settings
+
+        self.settings = settings


Is this part of the deprecation compatibility layer?
If so, please mark it accordingly so we know when to remove again.

Yes, will do

Django deprecated the use of DEFAULT_FILE_STORAGES and STATICFILES_STORAGE in v4.2 and removed it in v5.0. It provides a compat layer in 4.2, so even if the user set this value using the legacy key, the value will be available in django.conf.settings as: * settings.STORAGES['default']['BACKEND'] # default storage * settings.STORAGES['staticfiles']['BACKEND'] # staticfiles storage This commit updates the codebase to use the new value.

mdellweg

Feel free to merge.
I was just wondering if the user facing changelog should be moved to the decprecations section (It includes a deprecation for 3.85).

pedro-psb · 2025-01-09T14:13:16Z

I'll do that. I didnt want to use .removal because its not removed yet, but this better conveys the intent.

mdellweg · 2025-01-09T14:17:47Z

If you look, the title for ".removal" will be "Deprecations and Removals".

Co-authored-by: Matthias Dellweg <2500@gmx.de>

github-actions bot added multi-commit no-changelog no-issue labels Nov 20, 2024

pedro-psb force-pushed the update-deprecated-django-storage-config branch 4 times, most recently from ba1fb1a to fb6f05d Compare November 21, 2024 21:19

pedro-psb changed the title ~~Update deprecated django storage config~~ [PULP-210] Update deprecated django storage config Nov 28, 2024

pedro-psb force-pushed the update-deprecated-django-storage-config branch 13 times, most recently from 3890c05 to e9be44c Compare December 13, 2024 20:59

pedro-psb force-pushed the update-deprecated-django-storage-config branch 5 times, most recently from 6aa2681 to 94f262b Compare December 17, 2024 19:47

pedro-psb marked this pull request as ready for review December 17, 2024 19:47

pedro-psb mentioned this pull request Dec 17, 2024

Add docs about STORAGES setting #6158

Draft

mdellweg reviewed Dec 18, 2024

View reviewed changes

pedro-psb force-pushed the update-deprecated-django-storage-config branch from 94f262b to 6be238c Compare December 18, 2024 14:58

mdellweg reviewed Dec 18, 2024

View reviewed changes

pedro-psb force-pushed the update-deprecated-django-storage-config branch from 6be238c to 5a4dcc0 Compare December 18, 2024 15:46

mdellweg reviewed Jan 2, 2025

View reviewed changes

pedro-psb force-pushed the update-deprecated-django-storage-config branch from 347da3b to 1dc0342 Compare January 2, 2025 17:00

pedro-psb force-pushed the update-deprecated-django-storage-config branch 2 times, most recently from 1b6b66a to c34c168 Compare January 8, 2025 19:46

mdellweg previously approved these changes Jan 9, 2025

View reviewed changes

pedro-psb and others added 2 commits January 9, 2025 11:56

Add compat layer to enable using STORAGES

3e3492a

Co-authored-by: Matthias Dellweg <2500@gmx.de>

Update CI settings to partially use new storage settings

d771a25

pedro-psb dismissed mdellweg’s stale review via d771a25 January 9, 2025 14:57

pedro-psb force-pushed the update-deprecated-django-storage-config branch from c34c168 to d771a25 Compare January 9, 2025 14:57

pedro-psb enabled auto-merge (rebase) January 9, 2025 15:58

mdellweg approved these changes Jan 9, 2025

View reviewed changes

pedro-psb merged commit 2256ff4 into pulp:main Jan 9, 2025
12 checks passed

pedro-psb deleted the update-deprecated-django-storage-config branch January 9, 2025 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PULP-210] Update deprecated django storage config #6058

[PULP-210] Update deprecated django storage config #6058

pedro-psb commented Nov 20, 2024 •

edited

Loading

mdellweg Dec 18, 2024

pedro-psb Dec 18, 2024

mdellweg Dec 18, 2024

pedro-psb Dec 18, 2024 •

edited

Loading

mdellweg Dec 18, 2024

mdellweg Dec 18, 2024

pedro-psb Dec 18, 2024 •

edited

Loading

mdellweg left a comment

mdellweg Dec 18, 2024

mdellweg Dec 18, 2024

mdellweg Jan 2, 2025

pedro-psb Jan 2, 2025

mdellweg Jan 2, 2025

pedro-psb Jan 2, 2025

mdellweg left a comment

pedro-psb commented Jan 9, 2025

mdellweg commented Jan 9, 2025

		storages._backends = settings.STORAGES.copy()
		storages.backends

		setattr(global_settings, "DEFAULT_FILE_STORAGE", _DEFAULT_FILE_STORAGE)
		setattr(global_settings, "STORAGES", _STORAGES)

		@@ -0,0 +1 @@
		Removed `DEFAULT_FILE_STORAGE` setting in favour of the new `STORAGES` options introduced in Django 4.2.

	For compatibility, plugins should replace access to:
	For compatibility, plugins must replace access to:

		These legacy settings were deprecated in Django 4.2 and will be removed in Pulp 3.85 or Pulp 4,
		whichever comes first.

		@@ -11,7 +11,6 @@

		from pulpcore.tests.functional.utils import get_files_in_manifest, download_file

		from pulpcore.app import settings

[PULP-210] Update deprecated django storage config #6058

[PULP-210] Update deprecated django storage config #6058

Conversation

pedro-psb commented Nov 20, 2024 • edited Loading

Summary

Open questions:

Plugins TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-psb Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedro-psb Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

mdellweg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdellweg left a comment

Choose a reason for hiding this comment

pedro-psb commented Jan 9, 2025

mdellweg commented Jan 9, 2025

pedro-psb commented Nov 20, 2024 •

edited

Loading

pedro-psb Dec 18, 2024 •

edited

Loading

pedro-psb Dec 18, 2024 •

edited

Loading