msimerson / Mail-Toaster-6

Mail Toaster 6
https://github.com/msimerson/Mail-Toaster-6/wiki
BSD 3-Clause "New" or "Revised" License
46 stars 16 forks source link

Allow to set Roundcube product_name setting using ROUNDCUBE_PRODUCT_NAME #521

Closed tuffnatty closed 1 year ago

tuffnatty commented 1 year ago

Changes proposed in this pull request:

Checklist:

msimerson commented 1 year ago

probably should document this (with default setting) in mail-toaster.conf, yes?

tuffnatty commented 1 year ago

I am not sure if it's good to clutter mail-toaster.sh with app-specific settings in the long run, but since there is no other mechanism yet, probably yes. Added this and proper quoting for the user-supplied value.

msimerson commented 1 year ago

I am not sure if it's good to clutter mail-toaster.sh

One alternative could be a config section at the top of each provision/*.sh script. Anchor it with start and end tags so that the provisioned values can be displayed for a couple seconds before the service is provisioned.

tuffnatty commented 1 year ago

Did you have something like this in mind?

msimerson commented 1 year ago

Yeah.

Just thinking out loud here. If the ROUNDCUBE_SQL and ROUNDCUBE_DEFAULT_HOST settings were also moved into provision/roundcube.sh, they'd still get exported and settings honored if they existed (for legacy installs) in mail-toaster.conf. But scripts like provision/mysql that check for a case like this:

if [ "$TOASTER_MYSQL" = "1" ] || [ "$SQUIRREL_SQL" = "1" ] || [ "$ROUNDCUBE_SQL" = "1" ]; then

Would need to check the provision/* scripts to extract such settings.

Hmmm, a showstopper for this idea might be that provision/* files are ephemeral and get updated from the mt6 repo (if newer) every time they're run. So storing settings in there wouldn't be persistent.

Thoughts?

Perhaps something like ./conf.d would be better.

OTOH, there aren't that many knobs to twiddle yet, and having them all in mail-toaster.conf, which is persistent, works fairly well.

tuffnatty commented 1 year ago

My idea was not storing the settings in provision scripts, just set the default values from there, display them, and if user needs to customize them, he can just add them to (already generated) mail-toaster.conf. As for the provision/mysql.sh, at the first sight I do not see much sense in this check. Just to prevent a user from creating an extra jail by thoughtlessly copy&pasting commands from the wiki?

msimerson commented 1 year ago

My idea was not storing the settings in provision scripts, just set the default values from there, display them, and if user needs to customize them, he can just add them to (already generated) mail-toaster.conf.

I see. That does however make discoverability of settings harder. Right now, a default mail-toaster.conf is created with all the knobs available to twiddle in one place, because they're all set up mail-toaster.sh. That's not possible if they're in ./provision/* because those files only populate upon use.

Just to prevent a user from creating an extra jail by thoughtlessly copy&pasting commands from the wiki?

Yes, but more for CI testing, that builds through all the recipes in order. It's handy for small instances to not have heavy daemons like mysql started.

tuffnatty commented 1 year ago

Right now, a default mail-toaster.conf is created with all the knobs available to twiddle in one place, because they're all set up mail-toaster.sh.

It's not the case for CLAMAV_UNOFFICIAL, NICTOOL_VER, PHP_LISTEN_MODE, RSPAMD_SYSLOG, TLS_LIBRARY (which seem to accept values from outer environment if set). So I see your point on hard discoverability.

OTOH, there aren't that many knobs to twiddle yet, and having them all in mail-toaster.conf, which is persistent, works fairly well.

I have many more knobs in my mail-toaster.conf and provision scripts. That's why adding them all to mail-toaster.sh frightens me (although not many are fit for publishing).

tuffnatty commented 1 year ago

So, should I return to https://github.com/msimerson/Mail-Toaster-6/commit/d894517b88d36ed33f6164cf9cb0344f9f820a95 ? "Cluttering" mail-toaster.sh looks like a good tradeoff for the time being.

msimerson commented 1 year ago

So, should I return to d894517 ?

Yeah, I think that's for the best. For now. When/if mail-toaster.conf starts getting really hairy and ugly, a solution can be devised then.

msimerson commented 1 year ago

Although, leaving these in I think is still welcome, agree?

 echo "Configured Roundcube settings:"
 set | grep -E '^(ROUNDCUBE_DEFAULT_HOST|ROUNDCUBE_PRODUCT_NAME|ROUNDCUBE_SQL)='
 sleep 2

or perhaps simpler still:

 echo "Configured Roundcube settings:"
 set | grep -E '^ROUNDCUBE'
 sleep 2
tuffnatty commented 1 year ago

Done.