Issue #3589
closed
Settings in server.yaml have inconsistent names
Description
Some entries are all uppercase (SERVER_KEY, MEDIA_ROOT, etc) while others are lowercase (working_directory, databases, etc).
Related issues
Updated by daviddavis almost 6 years ago
- Related to Refactor #3151: Pulp3 server.yaml configuration names should be standardized added
Updated by daviddavis almost 6 years ago
- Related to Issue #3588: Add MEDIA_ROOT option section to server.yaml added
Updated by CodeHeeler almost 6 years ago
- Triaged changed from No to Yes
- Sprint set to Sprint 36
The naming convention for YAML is to use lowercase_with_underscores, so change to this throughout.
Updated by CodeHeeler almost 6 years ago
- Status changed from NEW to ASSIGNED
- Assignee set to CodeHeeler
Updated by CodeHeeler almost 6 years ago
So, the naming convention for yaml is all lower_snake_case. However, Django settings expect UPPERCASE for some of the values like DEBUG and while we could add logic to places to account for this, for future proofing later additions to the yaml file not already existing or commented out in the yaml file, it would probably be wise to simply go with UPPERCASE in the yaml to maintain consistency with Django's settings.py expectations. This does not break anything for yaml and prevents bugs on the Django side.
Just to prevent going down this naming convention train of thought in the future and replicating research, we should also add a comment to the yaml file explaining the naming convention reasoning here.
Updated by daviddavis almost 6 years ago
Is having our code convert the yaml settings from lower_snake_case to UPPER_SNAKE_CASE an option?
Updated by daviddavis almost 6 years ago
Another question: is not using yaml an option?
Updated by CodeHeeler almost 6 years ago
Are we already?
dalley pointed this out earlier: https://github.com/pulp/pulp/blob/3.0-dev/pulpcore/pulpcore/app/settings.py#L271
Updated by dalley almost 6 years ago
Yup, I do think that's what our code is already doing.
The thing is, since our yaml file is really just a thin wrapper around Django's settings.py variables, it would be best to stick close to Django for the simple reason that it makes comparing the settings with the Django docs easier.
Updated by daviddavis almost 6 years ago
The thing is, since our yaml file is really just a thin wrapper around Django's settings.py variables
I disagree. We have a number of other settings besides the ones we pass to Django like for our redis config, media root, content app, etc.
That said, it doesn't look like there's a prescriptive format for yaml though.
Updated by dalley almost 6 years ago
MEDIA_ROOT is a django setting though?
https://docs.djangoproject.com/en/2.0/ref/settings/#media-root
Updated by daviddavis almost 6 years ago
Sorry, I meant working directory:
https://github.com/pulp/pulp/blob/3.0-dev/pulpcore/pulpcore/etc/pulp/server.yaml#L93
Added by CodeHeeler almost 6 years ago
Added by CodeHeeler almost 6 years ago
Revision ffd8e225 | View on GitHub
Consistent names in server.yaml
The naming convention (not required) for yaml is lowercase_snake. We had some ALL_UPPERCASE names mixed in and wanted consistency. Due to some Django expectations, nested dicts, etc, we chose to make the names in server.yaml ALL_UPPERCASE with the exception of the logging section as Python is expecting lowercase there.
Updated by CodeHeeler almost 6 years ago
- Status changed from ASSIGNED to MODIFIED
Applied in changeset pulp|ffd8e2254cb5cecab3acf7bb7b636e59694757d2.
Updated by bmbouter over 4 years ago
- Status changed from MODIFIED to CLOSED - CURRENTRELEASE
.
Consistent names in server.yaml
The naming convention (not required) for yaml is lowercase_snake. We had some ALL_UPPERCASE names mixed in and wanted consistency. Due to some Django expectations, nested dicts, etc, we chose to make the names in server.yaml ALL_UPPERCASE with the exception of the logging section as Python is expecting lowercase there.
fixes #3589 https://pulp.plan.io/issues/3589