Alrighty - tests are currently running locally, but I think I finished the analysis and implementation for this one. Thank you for the feedback @casperklein and @polarathene - as always.

I have split everything into digestible commits, please review commit by commit.

@polarathene In your #4323 (comment), I have ticked what I implemented, and I explained what I had to do differently with comments in my commits.

Summary of change requests from review feedback. Not everything needs to be addressed, but the env export needs to be resolved either as described below or by reverting that change (it doesn't look like ENV can be adjusted for restarts, that results in a fresh container AFAIK, thus may not be a bug to restrict it to _setup())

• __environment_variables_export is a regression now. To prevent that you'll need to:
  • Migrate _check_log_level() from check-stack.sh into variables-stack.sh
  • Migrate the feature flag co-located ldap, oauth, saslauthd variable-stack.sh method calls from _register_functions (start-mailserver.sh) to a new method in variable-stack.sh with equivalent feature flag ENV checks.
  • Have the two methods (log-level check and feature-flag) called prior to the export method.

Done

• _setup_adjust_state_permissions and _setup_directory_and_file_permissions are both suitable to be managed during _check, but I won't enforce this change request for the PR.
  • NOTE: _setup_adjust_state_permissions is partially dependent upon _setup_save_states on a fresh container start should the volume not already provide the service dir for a feature. Running _setup_save_states in _check is also valid AFAIK, but the split is of mail_state.sh into two methods is still good to have.

I raised my concerns and problems with this in #4323 (comment).

• _setup_adjust_state_permissions() should inverse the debug log condition, as suggested. I don't see much value in debug log for notifying the user of skipping the logic, as per review feedback.

Done (I think)

From prior feedback:

• Optionally consider dropping _early_supervisor_setup() from start-mailserver.sh and moving that method from setup-stack.sh to daemons-stack.sh where _start_daemons() can call it before the for loop.

I do not think this would be a viable approach, as the script calls exit, which I'd like to call as early as possible, and not after the setup stack has already completed.

New commits begin here: 70eedac

I applied the changes and appeased the linting gods.

Some update from my side: This went from a small change into something bigger + a lot of discussion which I haven't fully read yet. To make a proper review, I'll have to take some time. At the moment, that's a bit challenging for me. I'd like to review asap, but unfortunately I can't give an ETA right now.
If this can wait some more, I am happy to review. Otherwise I try to review afterwards. I'll leave it to you both, when this gets merged 👍

If this can wait some more, I am happy to review. Otherwise I try to review afterwards. I'll leave it to you both, when this gets merged

I'm in no rush, I'll be working on the podman PR in the meantime 👍

PR Overview

This went from a small change into something bigger + a lot of discussion which I haven't fully read yet.

If it helps here is a quick overview for the diff.

• Primary change is in start-mailserver.sh to adjust the restart logic, it's mostly unified now so that it only calls _setup on fresh containers.
• Additional changes are more house-keeping related, some supportive to the primary change. A bulk of the diffs aren't as large as they seem, just the shuffling of lines and indentation adjustments.

Primary change is adjusting the restart check for /CONTAINER_START to this:

_early_supervisor_setup
_early_variables_setup

_log 'info' "Welcome to docker-mailserver ${DMS_RELEASE}"

_register_functions
_check

# Ensure DMS only adjusts config files for a new container.
# Container restarts should skip as they retain the modified config.
if [[ -f /CONTAINER_START ]]; then
  _log 'info' 'Container was restarted. Skipping most setup routines.'
  # We cannot skip all setup routines because some need to run _after_
  # the initial setup (and hence, they cannot be moved to the check stack).
  _setup_directory_and_file_permissions
  _setup_adjust_state_permissions
else
  _setup
fi

• Effectively now a restart runs everything except _setup.
• There are two _setup methods that still need to be run for restarts, but were proposed to be migrated to _check functions.
  • _setup_directory_and_file_permissions() is a bunch of misc cleanup operations. One of those is to remove PID files (primary regression we're fixing by ensuring this method is run), and also ensures /var/mail ownership is corrected if necessary.
  • _setup_adjust_state_permissions() is a new method in mail_state.sh, which was split into two separate methods. This is the 2nd method for mail_state.sh which just manages ownership/permission fixes, while the earlier portion only runs on fresh containers to create/copy and symlink to the DMS state volume (if any ENV is changed to toggle a feature, Docker will force recreate the container, however it looks like it's probably safe to still run this during restarts too, we effectively were prior to v14).
• @georglauterbach did attempt to migrate those methods to _check(), but ran into test failures with /var/mail ownership. I haven't looked into reproducing that yet. That would simplify the conditional block, removing the two separate calls for restart there and also their repeated use in _setup. I'm not too fond of the two call sites and would prefer to simplify, but not blocking the PR on that.

Additional changes:

• STATEDIR in mail_state.sh was renamed to DMS_STATE_DIR, along with an adjust to the conditional and related log and that is repeated with the 2nd method.
  • Part of this change removed the indent of the method as it previously was all wrapped in the conditional (that's now inverted to exit early).
  • It also introduced __declare_readonly(), which personally I don't feel is necessary and adds some complexity, but I can understand why @georglauterbach wanted to introduce it (this is more of a DX issue with using bash though). It could be avoided and DMS_STATE_DIR could just be declared at the top of mail_state.sh.
• While the diff is large/noisy for mail_state.sh actual changes is minimal and it's effectively white-space. I did add some minor housekeeping changes but dropping a variable name DESTDIR that was only used for mkdir -p, preferring to inline that and add a comment instead. Likewise for a chcon call, I had the comment append a reference link in favor over relying on git blame for context.
• As part of the earlier restart logic changes, _setup() was making calls to setup ENV vars via variables-stack.sh but most of these were not registered as part of _setup(), just co-located with the equivalent feature. These have instead been extracted out and managed via variables-stack.sh at an earlier stage as part of _early_variables_setup().
• _check_log_level() (check-stack.sh) was migrated to variables-stack.sh as __environment_variables_log_level(), which is a house-keeping thing rather than addressing anything in particular, the logic was ENV related so it belonged in variables-stack.sh instead.

Indeed, this has become bigger than I anticipated as well. I'd still like to merge ASAP TBH, because I'd really like publish v15.0.0.

Here is my proposal: I'd merge this and prepare the v15 release. We'll pick 1.5 weeks for the freeze because of this change. Please let me know whether this is okay for you both.

@polarathene thanks for the overview :) I added the __declare_readonly to ensure we do not run into problems with readonly variables being defined more than once (e.g. if this script is ever used while the environment was already loaded). It's really a Bash thingy, I guess.. readonly needs special precautions.

I have resolved the merge conflicts with master and enabled autp-merge now.

@georglauterbach Good structured commits 👍
@polarathene Thank you for the summary, that helped me to understand things (faster). 👍

The only nitpick I have is the "DMS_STATE_DIR" change:

• Why making it global when not needed?
• DMS_STATE_DIR is set explicitly, why is this check needed? {DMS_STATE_DIR:?DMS_STATE_DIR is not set}. This should never be unset?
• do we really need __declare_readonly?

I just think, that this makes things unnecessary complicated. Like @polarathene said, DMS_STATE_DIR could just be declared at the top of mail_state.sh should be enough.

Edit: Side effect of __declare_readonly I run into while reviewing is, I couldn't easily search for the variable definition of DMS_STATE_DIR. I usually search for "VAR=" to find the location of a variable definition. Sure, not a big thing, I just wanted to mention it.

Why making it global when not needed?

I felt the same way but I didn't want to nitpick heavily on it.

I understand the intention to minimize caveats from using Bash. It's one of the main friction points we have with maintaining the project when variables / methods are being pulled into scope from other scripts, something we shouldn't have to feel like we're walking on glass with.

If we do explore migrating some scripts away from bash at some point, that's one of the main benefits. So far NuShell seems like a good compromise for that (a rust-based shell that provides optional type annotation, structured data, module imports, etc). I understand wanting to avoid Python and concerns with languages that'd involve compilation vs an interpreter for scripts, Nu might be a good candidate for such. We've had this discussion quite a bit so I won't drag it out again here 😅

I do agree that I'm not fond of the added complexity / abstraction to add preventative measures prematurely though.

I will remove this part again after seeing your feedback :)