Skip to content
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

Create empty local_settings.py when none exists to avoid erronious "missing local_settings" warning #21890

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Feb 19, 2024

Relates to: mozilla/addons#1992

Description

modify settings.py to create a local_settings.py with base_settings import if none exists. This ensures the git ignored local_settings file is always present without polutting it with any contents.

Context

If you don't have a local_settings.py file, any command that relies on settings.py (almost anything) will print a warning that it could not find the file.

Testing

You will need to build the container locally and shell into it. I was able to do this with

docker build -t test_locale .   

Our image doesn't have a command or entry point so you need to define one when running it.

docker run -d --name test_locale_container test_locale tail -f /dev/null

ANd finally shell

docker exec -it test_locale_container bash 

NOTE: There is no volume, so if you change the code on your machine, it will not be reflected in the container

Now you can run any command that uses the settings.py file

make update_assets

You should not see any warnings about missing local settings file, even if you delete the file and rerun. If you have a local_settings file with a setting, it should not update it.

@KevinMind KevinMind force-pushed the ADDSRV-720-local-settings branch 2 times, most recently from b1a42bb to 25bd5ae Compare February 19, 2024 17:48
@KevinMind KevinMind requested review from diox and eviljeff February 19, 2024 17:49
@KevinMind KevinMind force-pushed the ADDSRV-720-local-settings branch from 25bd5ae to 6678730 Compare February 20, 2024 11:27
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine but I'm not exactly sure what this solves - at the very least the PR title should be more descriptive.

local_settings.py was always optional - we had a warning if it didn't exist, but that's it.

settings.py is the default setting file, you can see it referenced by the os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'settings') call in manage.py.

  • dev/stage/prod override that DJANGO_SETTINGS_MODULE environment variable with their own and don't rely on it at all, importing settings_base.py directly.
  • local envs don't do anything special to settings.py is the de-facto development settings file, as mentioned in the docstring at the top.
  • tests pass DJANGO_SETTINGS_MODULE=settings_tests but settings_tests.py does include settings for convenience.

@KevinMind
Copy link
Contributor Author

The PR started out as attempting to both ensure the local_settings file exists and remove the need to specify it in the Dockerfile here but I didn't realize yet that creating a local_settings that inherits base_settings totally messes up the final settings, probably based on the dependency ordering.

So this PR is much smaller now, scratching my itch of not wanting to see that warning "allllll the time" This just makes sure the empty file exists if it doesn't already and makes the warning go away. Still would be a nice to have IMO as I strongly dislike that warning.

@KevinMind KevinMind requested a review from diox February 20, 2024 17:16
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs a better title to reflect what it does

settings.py Outdated Show resolved Hide resolved
@KevinMind KevinMind changed the title ADDSRV-720-local-settings Create empty local_settings.py when none exists to avoid erronious "missing local_settings" warning Feb 21, 2024
Copy link
Member

@diox diox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ with the fix to codestyle (probably caused by my suggestion, sorry about that)

@KevinMind KevinMind force-pushed the ADDSRV-720-local-settings branch from 7dd1a97 to 9300577 Compare February 23, 2024 08:30
@KevinMind KevinMind merged commit 10fe3e1 into master Feb 26, 2024
10 checks passed
@KevinMind KevinMind deleted the ADDSRV-720-local-settings branch February 26, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants