aleph icon indicating copy to clipboard operation
aleph copied to clipboard

FEATURE: Avoid setting helpful defaults for environment variables

Open brassy-endomorph opened this issue 3 years ago • 1 comments

Is your feature request related to a problem? Please describe.

There are multiple places where environment variables have defaults set and especially places where defaults are set for otherwise disabled features. This is confusing and leads to unexpected errors when the application is misconfigured.

There ae defaults in these places (that I've found):

https://github.com/alephdata/aleph/blob/e06f94ce1eed2b9230b9c6ec7697b99a68c1046d/Dockerfile#L40-L47

https://github.com/alephdata/aleph/blob/e06f94ce1eed2b9230b9c6ec7697b99a68c1046d/aleph/settings.py#L112-L114

And these email settings are particularly annoying. If they're not set in the env vars, the should not be present in the module as that is very unexpected behavior.

https://github.com/alephdata/aleph/blob/e06f94ce1eed2b9230b9c6ec7697b99a68c1046d/aleph/settings.py#L152-L159

And in this case, the ES URI is set in two places (to the same value but still):

https://github.com/alephdata/aleph/blob/e06f94ce1eed2b9230b9c6ec7697b99a68c1046d/aleph/settings.py#L168-L169

Describe the solution you'd like

If an environment variable isn't set (e.g., env.get("VAR") is None, then it should not be assigned to a variable in settings.py.

Describe alternatives you've considered

This was probably done to simplify dev in the docker-compose.yml or to simplify configuration with sensible defautls, but some values should just error out if they're missing. As in, if an admin never set the ES URI then the app should crash hard with "missing config value" and not "host not found" or "connection refused".

Additional context

All the env vars should really be documented in a single place in the docs including their value, what they might map to in flask land, whether they're required, their default value if applicable, and a brief description of what they do or not. The current configuration section of the docs is not sufficient for an admin to figure out how to configure the app without digging through the code.

This relates to #2435.

brassy-endomorph avatar Aug 01 '22 07:08 brassy-endomorph

This was the direct cause of #2428

brassy-endomorph avatar Aug 01 '22 09:08 brassy-endomorph