Related T1533
Details
Details
- Reviewers
douardda olasd - Group Reviewers
Reviewers - Commits
- rDDEPd78aa0089d7d: settings.prod: Use SWH_CONFIG_FILENAME to load & check swh config
Diff Detail
Diff Detail
- Repository
- rDDEP Push deposit
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/79/ for more details.
| swh/deposit/wsgi.py | ||
|---|---|---|
| 17 | Shouldn't this be dropped? | |
| swh/deposit/wsgi.py | ||
|---|---|---|
| 17 | That's a good question. And i think you are right. I'm unsure then about the config_file parameter of the function below though. | |
| swh/deposit/wsgi.py | ||
|---|---|---|
| 17 |
I mean does that even make sense to open it up at all? context: production setup is to use that module as entry point [1] | |
Comment Actions
From what I can tell, in your make_app_from_configfile function, the conf object is never used, only checked. That feels very brittle to me and I think the checks should happen in the swh.deposit.settings.production module directly.
It's standard practice in Django deployments to rely on environment variables set outside the WSGI process manager to load configuration; that's what the DJANGO_SETTINGS_MODULE environment variable is there for. I think it makes sense for our production settings module to also look at the SWH_CONFIG_FILENAME envvar to pull the settings that we don't want to publish in the main DJANGO_SETTINGS_MODULE (which is itself published in the swh.deposit module on PyPI).
I'm not too fussed about the wsgi module shipping a default value for DJANGO_SETTINGS_MODULE (as it's not supposed to be changed anyway), and frankly for SWH_CONFIG_FILENAME as well as it makes for easier deployment; but it's not too hard to set SWH_CONFIG_FILENAME explicitly on the gunicorn service declaration instead (via an Environment directive in the systemd unit).
| swh/deposit/wsgi.py | ||
|---|---|---|
| 17 | When the gunicorn executable parameter is set to a plain module, it's going to look for an attribute called application. It's also possible to explicitly set an explicit object to look for | |
Comment Actions
From what I can tell, in your make_app_from_configfile function, the conf object is never used, only checked.
True.
That feels very brittle to me and I think the checks should happen in the swh.deposit.settings.production module directly.
ah, yeah, makes sense, that's where it's used anyway!
It's standard practice in Django deployments to rely on environment variables set outside the WSGI process manager to load configuration; that's what the DJANGO_SETTINGS_MODULE environment variable is there for. I think it makes sense for our production settings module to also look at the SWH_CONFIG_FILENAME envvar to pull the settings that we don't want to publish in the main DJANGO_SETTINGS_MODULE (which is itself published in the swh.deposit module on PyPI).
Yes, right.
I think that's what i aimed at in D1161.
I'm not too fussed about the wsgi module shipping a default value for DJANGO_SETTINGS_MODULE (as it's not supposed to be changed anyway), and frankly for SWH_CONFIG_FILENAME as well as it makes for easier deployment; but it's not too hard to set SWH_CONFIG_FILENAME explicitly on the gunicorn service declaration instead (via an Environment directive in the systemd unit).
Right, well, i already did the change in the manifest so i'll keep that ;)
Thanks, that helps a lot.
Comment Actions
Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/84/ for more details.