linuxserver / docker-healthchecks

GNU General Public License v3.0
177 stars 37 forks source link

Improve env var handling and cont-init.d/30-config #73

Closed kaysond closed 2 years ago

kaysond commented 3 years ago

linuxserver.io



Description:

This PR is a work in progress that addresses some issues in the generation of local_settings.py. Mainly, it prevents duplicate lines in the files when REGENERATE_SETTINGS is enabled, and it removes the defaults for email username/pass env vars.

Details

I ended up removing all of the default variable settings from the script. My rationale here is two-fold: first, most of the defaults are totally useless anyways since they have to be specific to a user's setup. Second, healthchecks (or django) itself provides defaults for all of the variables anyways, so it seems better to use the app's defaults rather than inject our own, in case they change. These are set up in healthcheck's settings.py (see link below).

I added a function that scans for a setting and either adds or replaces it in the local_settings.py file and modified the for loop to properly handle both booleans and arrays (i.e. ALLOWED_HOSTS - also improved documentation for it). Removed the PING_ENDPOINT line because healthchecks already sets a default for this from SITE_ROOT, but users can still override with an env var.

If SECRET_KEY doesn't exist as an env var or in the config file, one is generated and inserted into the config file (used /dev/urandom for generation based on a snippet from stack overflow; feel free to suggest an alternate method)

Benefits of this PR and context:

This addresses #69 and #70. I also took the liberty of including #61 since that seems to be dead.

How Has This Been Tested?

Not yet fully tested

Source / References:

https://github.com/healthchecks/healthchecks/blob/master/hc/settings.py

kaysond commented 3 years ago

@homerr This is still a work in progress, and I haven't finished testing everything. It ended up being a bit more involved of a change than I initially expected so I wanted to get some feedback.

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/healthchecks/v1.22.0-pkg-62b9cb81-pr-73/index.html https://ci-tests.linuxserver.io/lspipepr/healthchecks/v1.22.0-pkg-62b9cb81-pr-73/shellcheck-result.xml

kaysond commented 3 years ago

Added the rest of the env vars from hc docs so they're saved in local_settings.py as well.

Ran the following tests:

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/healthchecks/v1.22.0-pkg-62b9cb81-pr-73/index.html https://ci-tests.linuxserver.io/lspipepr/healthchecks/v1.22.0-pkg-62b9cb81-pr-73/shellcheck-result.xml

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/healthchecks/v1.22.0-pkg-62b9cb81-pr-73/index.html https://ci-tests.linuxserver.io/lspipepr/healthchecks/v1.22.0-pkg-62b9cb81-pr-73/shellcheck-result.xml

kaysond commented 2 years ago

Pinging some folks for a review @Roxedus @homerr @alex-phillips

kaysond commented 2 years ago

Another bump for @alex-phillips review

obvionaoe commented 2 years ago

please review and merge this, as without this PR the ALLOWED_HOSTS env var is completely broken @alex-phillips

alex-phillips commented 2 years ago

This looks good to me. Thanks and sorry for the delay.