Project

Profile

Help

Issue #3589

Settings in server.yaml have inconsistent names

Added by daviddavis almost 3 years ago. Updated about 1 year ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Sprint:
Sprint 37
Quarter:

Description

Some entries are all uppercase (SERVER_KEY, MEDIA_ROOT, etc) while others are lowercase (working_directory, databases, etc).


Related issues

Related to Pulp - Refactor #3151: Pulp3 server.yaml configuration names should be standardized CLOSED - NOTABUG

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Related to Pulp - Issue #3588: Add MEDIA_ROOT option section to server.yamlCLOSED - CURRENTRELEASE<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

Associated revisions

Revision ffd8e225 View on GitHub
Added by CodeHeeler over 2 years ago

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

Revision ffd8e225 View on GitHub
Added by CodeHeeler over 2 years ago

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

History

#1 Updated by daviddavis almost 3 years ago

  • Related to Refactor #3151: Pulp3 server.yaml configuration names should be standardized added

#2 Updated by daviddavis almost 3 years ago

  • Related to Issue #3588: Add MEDIA_ROOT option section to server.yaml added

#3 Updated by CodeHeeler almost 3 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.

#4 Updated by CodeHeeler over 2 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to CodeHeeler

#5 Updated by rchan over 2 years ago

  • Sprint changed from Sprint 36 to Sprint 37

#7 Updated by CodeHeeler over 2 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.

#8 Updated by daviddavis over 2 years ago

Is having our code convert the yaml settings from lower_snake_case to UPPER_SNAKE_CASE an option?

#9 Updated by daviddavis over 2 years ago

Another question: is not using yaml an option?

#11 Updated by dalley over 2 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.

#12 Updated by daviddavis over 2 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.

#13 Updated by dalley over 2 years ago

#15 Updated by CodeHeeler over 2 years ago

  • Status changed from ASSIGNED to MODIFIED

#16 Updated by bmbouter over 1 year ago

  • Tags deleted (Pulp 3)

#17 Updated by bmbouter about 1 year ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Please register to edit this issue

Also available in: Atom PDF