mailinabox icon indicating copy to clipboard operation
mailinabox copied to clipboard

Allow user to overwrite config using `$STORAGE_ROOT/mailinabox.conf`

Open bilogic opened this issue 1 year ago • 5 comments

Based on comments from https://github.com/mail-in-a-box/mailinabox/pull/2349#issuecomment-1987193731, this PR attempts to provide a convention on how mailinbox determines its configuration

  1. mailinbox comes with defaults stored in /etc/mailinabox.conf
  2. Everything in 1 can be overwritten by defining it again in $STORAGE_ROOT/mailinabox.conf
  3. Backwards compatible If $STORAGE_ROOT/mailinabox.conf does not exists

To make it easier for the reviewer(s),

  • All code changes are found in https://github.com/mail-in-a-box/mailinabox/pull/2382/commits/a7beb721680d405f86567ba001caecdb5eb2fe7e, which tries to read from $STORAGE_ROOT/mailinabox.conf into DEFAULT_xxx

I hope this can be accepted so that I can further proceed with introducing a configurable TTL and DKIM_SELECTOR

#2352 #2383

bilogic avatar Apr 21 '24 09:04 bilogic

What is the purpose of having multiple files?

additionally, there are dozens of uses of /etc/mailinabox.conf in the codebase that would not use changes in $STORAGE_ROOT/mailinabox.conf.

Syntax changes to code only masks git blame's and are likely not needed in a change to $STORAGE_ROOT/mailinabox.conf

jvolkenant avatar Apr 23 '24 19:04 jvolkenant

What is the purpose of having multiple files?

For a lack of better word, I will call it layering (or inheritance), the algo goes like this:

  • read /etc/mailinabox.conf as default, out-of-box (OOB) settings
  • customize OOB settings by overwriting with whatever is inside $STORAGE_ROOT/mailinabox.conf

VSCode loads its settings this way, and I found to be very flexible for all kinds of use cases And it is easy to tell what are the defaults, and what the user has changed

additionally, there are dozens of uses of /etc/mailinabox.conf in the codebase that would not use changes in $STORAGE_ROOT/mailinabox.conf.

Thanks for pointing this out, I missed it.. downside is, this PR not going to work

Syntax changes to code only masks git blame's and are likely not needed in a change to $STORAGE_ROOT/mailinabox.conf

Ok, I will omit them in future, but ideally we can pick a code formatter, this will allow folks like me to PR more easily and does not affect those who don't use a code formatter

bilogic avatar Apr 24 '24 02:04 bilogic

@jvolkenant

I had a quick look, only utils.py needs to have a slight change in logic and all *.sh references are

source setup/functions.sh # load our functions
source /etc/mailinabox.conf # load global vars

I believe this can be solved by combining load global vars into load our functions (and global vars)

Before I proceed, are there any other concerns about this approach?

bilogic avatar Apr 24 '24 02:04 bilogic

@dms00

Yes I agree that it breaks the blame part, a discussion here https://github.com/orgs/community/discussions/5033 led to GitHub's solution here https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view, so I will be taking out the formatting commits into another PR.

Apart from the formatting changes, are there other concerns? I appreciate an explicit No further concerns as the time I spend on each PR is fairly high, don't want to keep redoing it.

bilogic avatar Apr 25 '24 03:04 bilogic

This is a little too complicated for the problem. New options can be stored in STORAGE_ROOT anywhere without having to be in a merged configuration namespace with the settings in mailinabox.conf. The main purpose of mailinabox.conf is to set the location of STORAGE_ROOT and other system properties that aren't likely to be preserved when restoring a backup to a new machine.

JoshData avatar Dec 22 '24 13:12 JoshData