start-mailserver.sh split

[georglauterbach]

Description

I will be very low on time for the next month (this time for real :D), and decided to provide this project with a bit of "last" work before I do less here, and I will now spend more time other things. This is not to say I'm gone. If you tag me with an @ I will most likely still respond, but I won't overlook the tracker and PRs that active.

I was always concerned with the size of start-mailserver.sh - so I decided to split it up. The new scripts under target/scripts/startup/ are parts of the old big file. They are sourced at the end of the new start-mailserver.sh.

Now, the file setup.-stack.sh is still very large, but at least it's a closed context. I made sure to re-write what I deemed inappropriate for the style this project uses. I would like to see this rigorous style being enforced in the future too. But I admit I'm sure you @casperklein @wernerfred will do a good job. And I'm not completely gone either.

I would like to see this being merged before v9.0.0 is released. @wernerfred I guess you will be releasing v9.0.0 and writing the changelog when all PRs we mentioned in the maintainers team are merged.

What I would love to see in the future:

1. All scripts under target/bin/ should be able to support the new usage information
2. The tests will need a lot of refactoring, and Bats needs an update (which is coming, I think, soon)

Fixes

The enormous size of start-mailserver.sh.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change that does improve existing functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (README.md or ENVIRONMENT.md or the Wiki)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[casperklein]

I like the idea of the split. The only disadvantage/challenge I see is, that we add another thing to remember, when we introduce new ENVs.

To keep all consistent, we must ensure, that at least the following things are done:

• Add entry to ENVIRONMENT.md
• Add default value to target/scripts/start-mailserver.sh
• Add to function _setup_default_vars in target/scripts/startup/setup-stack.sh

[georglauterbach]

I like the idea of the split. The only disadvantage/challenge I see is, that we add another thing to remember, when we introduce new ENVs.

To keep all consistent, we must ensure, that at least the following things are done:

• Add entry to ENVIRONMENT.md

• Add default value to target/scripts/start-mailserver.sh

• Add in target/scripts/startup/setup-stack.sh

I thought about this too. I could easily put the function _setup_default_vars back in start-mailserver.sh. I will see how that turn out tomorrow, after fixing the conflicts.

[georglauterbach]

I came up with an elegant solution. Similar to what it was like before, I create a dictionary VARS which is just "played back" in _setup_default_vars. And since we are using := the default assignment to the variable itself happens and we can freely use them later as is with default values. I like this, as it eliminates the need to introduce variables in 3 places, as you @casperklein already mentioned.

[georglauterbach]

@casperklein what do you think now? From my side ready for merging.

[wernerfred]

I've not looked in detail for quotes and style etc but imho I can approve the logic your changes follow 👍🏻

[casperklein]

@aendeavor I will review it tomorrow. But from a quick check it looks good to me 👍

[wernerfred]

I would like to see this being merged before v9.0.0 is released. @wernerfred I guess you will be releasing v9.0.0 and writing the changelog when all PRs we mentioned in the maintainers team are merged.

So I will gather all changelog information when this get's merged and release v9.0.0 afterwards 🥳

[georglauterbach]

@wernerfred Perfect. This is exactly what I wanted to ask you now :D

@casperklein If you approve this PR, go ahead and squash merge it into master in order to enable the release of v9.0.0 :)

PS: @wernerfred Don't forget to update the README for version 9, especially the setup.sh download section

[casperklein]

No deal breaker, just a little styling question:

Before: ################### >> fix funcs

After: # ? >> Fixes

I was just wondering about the "?". What' s the meaning of it? Why not just # >> Fixes

[georglauterbach]

I was just wondering about the "?". What' s the meaning of it? Why not just # >> Fixes

When you're using the "Better Comments" extension some IDEs provide, comments followed by an ? will become blue:

[casperklein]

Ah colors.. another general question. You did a great afford in colorizing all the scripts.

echo "-e${3:-}" "[ \e[0;92mTASKLOG\e[0m ] ${2}" ;; # light green

Was there a reason, why you didn't use variables, e.g. \e[0;92m instead of $LGREEN

echo "-e${3:-}" "[ ${LGREEN}TASKLOG${COLOR_RESET} ] ${2}" ;; # light green

This looks IMHO more cleaner.

Maybe I provide a PR in mid-term to switch to variables. But before (wasting time), I want to ensure, there was no particular reason not to so.

[georglauterbach]

Was there a reason, why you didn't use variables, e.g. \e[0;92m instead of $LGREEN

I thought this was more compact:)

This looks IMHO more cleaner.

Feel free to change it. But until supervisor fixes its issues with Color, there is no difference :)