-
Notifications
You must be signed in to change notification settings - Fork 35
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.
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
Add flag to exercise django's STORAGES setting on CI #941
Add flag to exercise django's STORAGES setting on CI #941
Conversation
…config If plugins provide STORAGES in the s3 settings in the template config, these will be used intead of the default ones from the template.
MEDIA_ROOT: "" | ||
S3_USE_SIGV4 = True | ||
{% if "STORAGES" in pulp_scenario_settings %} | ||
STORAGES = {{ pulp_scenario_settings["STORAGES"] }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the | repr
the reason why you need to single out this line? Is this related to the "{{ minio_access_key }}"
part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I dont know, but maybe.
The reason was to keep the alternatives explicit
@@ -72,6 +72,7 @@ DEFAULT_SETTINGS = { | |||
"test_performance": False, | |||
"test_reroute": True, | |||
"test_s3": False, | |||
"test_storages_compat_layer": False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add s3 to that name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue this is an implementation detail, but we can also be more concrete and put something like use_new_storages_on_s3
if you prefer.
{% endfor %} | ||
{% endif %} | ||
|
||
{% if s3_test | default(false) %} | ||
MEDIA_ROOT: "" | ||
S3_USE_SIGV4 = True | ||
{% if "STORAGES" in pulp_scenario_settings %} | ||
STORAGES = {{ pulp_scenario_settings["STORAGES"] }} | ||
{% if test_storages_compat_layer %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new one, right so the "compat_layer" is the other one, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, compat layer is the code that enables both, so test compat layer means ensure both branches coverage (testing what was untested).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Let's not fuzz too much about that name. I hope we will remove it soon again.
The current version is tested here: pulp/pulpcore#6202 |
If plugins provide STORAGES in the s3 settings in the template config, these will be used intead of the default ones from the template.