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

Removed bakerydemo-settings-local.py.example #70

Merged
merged 1 commit into from
Nov 11, 2023
Merged

Conversation

jsma
Copy link
Contributor

@jsma jsma commented Nov 10, 2023

This file never worked, it caused a circular import loop since it attempts to from bakerydemo.settings.dev import * but the dev settings does:

try:
    from .local import *  # noqa
except ImportError:
    pass

Fixed the setup.sh script to set the correct settings module environment variable

Fixes #68

This file never worked, it caused a circular import loop since it attempts to `from bakerydemo.settings.dev import *` but the dev settings does:

```python
try:
    from .local import *  # noqa
except ImportError:
    pass
```

Fixed the `setup.sh` script to set the correct settings module environment variable

Fixes wagtail#68
@jsma jsma mentioned this pull request Nov 11, 2023
@saevarom
Copy link
Collaborator

Thanks @jsma !

If you remove this file a fresh install will no longer connect to the database, since the code to load the env variablies lives in the bakerydemo-settings-local.py.example. This was probably working for a previous install since the bakerydemo/settings/local.py was already populated with the necessary code.

I would suggest amending the bakerydemo-settings-local.py.example instead so that in only includes code for connecting to the database.

@@ -1,6 +0,0 @@
from bakerydemo.settings.dev import * # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should probably go

Comment on lines -2 to -6
import dj_database_url

# Override settings here
db_from_env = dj_database_url.config(conn_max_age=500)
DATABASES['default'].update(db_from_env)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is needed for fresh installs so that we have a working local.py.

@jsma
Copy link
Contributor Author

jsma commented Nov 11, 2023

The database connection setting is actually set here:

DATABASE_URL: "postgres://wagtail:changeme@db/wagtail"

bakerydemo's settings.base picks this environment variable up here:

if "DATABASE_URL" in os.environ:
    DATABASES = {"default": dj_database_url.config(conn_max_age=500)}

@saevarom
Copy link
Collaborator

Ok, I see that now that it is being picked up in he base settings. Your statement about this never working is wrong, though, since that code change is only 7 months old.

But anyway, thanks for your work on this. Will test again later.

@jsma
Copy link
Contributor Author

jsma commented Nov 11, 2023

I'm sorry if my word choice seemed insulting! bakerydemo's settings.dev has not changed in 9 months, it was doing from .local import * at the time bakerydemo-settings-local.py.example was introduced in this project. DATABASE_URL was added to the docker-compose.yml two years ago, so between this environment variable already existing and the try/except in settings.dev swallowing the import loop exception, the error was never noticed.

@saevarom
Copy link
Collaborator

No worries. Just be mindful we're building on people's volunteer work and mistakes get sometimes made. It happens.

I've tested this on an existing setup and a fresh setup and this works great, thank you!

@saevarom saevarom merged commit 849497e into wagtail:main Nov 11, 2023
1 check passed
@jsma jsma deleted the fix-68 branch November 11, 2023 22:41
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.

bakerydemo-settings-local.py.example is invalid and not used
2 participants