scripts: small refactorings #2485

georglauterbach · 2022-03-14T20:24:35Z

Description

The commit descriptions are somewhat detailed. It basically boils down to:

Cron scripts are now added with HEREDOCs
Minor style adjustments

Type of change

Improvement (non-breaking change that does improve existing functionality)

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 the documentation under docs/)
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

This was not actually needed (and not a good idea either - ask me how I know: "warning: error loading certificate chain: key at index 1 in /etc/dms/tls/key does not match the certificate" in my log due to my custom Postfix `main.cf`). It turns out the _setup_ssl call would just set the exact same files again, which does absolutely nothing (except messing with custom configurations...). I therefore removed this portion again. I left the checksum generation for the files in the `_monitored_files_checksums` function because restarting Postfix is desired. Removing the test is also fine because 1. it was flawed to begin with 2. the changedetector functionality of detecting file changes is already tested in another test

We now use HEREDOCs to add new files to cron. This is, after testing the alternatives of copying files, the best solution in my opinion. We have munite control over the contents (especially when the content of the cron files depends on environment variables). Moreover, we need not adjust the `Dockerfile`.

The job could fail if no new fresh updates are available. This is however not a reason to receive a new email telling me the job failed. This commit provides a solution.

After reading the docs on the exit code, we can do better.

This is a bigger commit - it reworks the whole log functionality. The old one was confusing and not adhering to common standards, i.e. not following convention over configuration. With this commit, we use the common standard log levels 4. TRACE 3. DEBUG 2. INFO 1. WARN 0. ERROR to better provide the information to users and maintainers. With this changes, `DMS_DEBUG` was removed and `LOG_LEVEL` introduced. `DMS_DEBUG` is not usefule anymore (obviously), and replaced by the new `LOG_LEVEL`, which defaults to `info`. All scripts and tests were adjusted to use the new format. There is a special log level (`always`) which is used only by `start-mailserver.sh` to show the welcome messages independent of the log level. I took some care when converting the "old" log levels. With this rework, it should be easier to identify problems in logs and provide users with a clear and straight forward log level variable which is easy to understand. This is not actually a breaking change, but I thought it is fitting for the next major release `v11`.

We need not write up to loggers, sourcing the already present one is fine. This simplifies the whole script.

github-actions · 2022-03-14T20:36:41Z

Documentation preview for this PR is ready! 🎉

Built with commit: 75531fd

polarathene

Good PR overall! 👍

Some minor change suggestions if you think any would be improvements.
Caution towards the SSL check-for-changes.sh change. It may regress support for other users without third-party customizations.
log.sh should have explicit error case, always is better suited to use the * fallback. Not sure why error is set as default LOG_LEVEL case to rely on * as fallback/highlighting?
Possible bug with one of your cron heredoc additions.

Feedback: Personal opinion - Not a fan of HEREDOC

Cron jobs to heredocs (relevant commit with commit message for context) is not something I'm fond of personally.

I prefer external files instead of embedded/inlined, ENV as a convenience isn't sufficient for me. We already write ENV to a file for other scripts to read and utilize. I'd rather cron jobs we add are all in a common location from a maintenance perspective, but I understand if the majority of maintainers prefer the heredoc approach instead and have no interest in fighting that decision.

Feedback: Log changes could have been covered in a separate PR

The log level change is rather large and seems it would have suited it's own PR rather than mixed in with the other changes covered (relevant commit covering the bulk of logging changes). I understand it's mostly isolated to the linked commit, but for review purposes separate PRs would have been nicer 😅

.github/ISSUE_TEMPLATE/bug_report.yml

docs/content/config/environment.md

docs/content/contributing/issues-and-pull-requests.md

mailserver.env

target/scripts/check-for-changes.sh

test/linting/lint.sh

test/mail_special_use_folders.bats

target/scripts/startup/setup-stack.sh

This reverts commit d5c7f21.

This reverts commit abf18e5.

…pt-enhancements

georglauterbach · 2022-03-15T13:31:51Z

Good PR overall! +1

Thanks!

Some minor change suggestions if you think any would be improvements.

Caution towards the SSL check-for-changes.sh change. It may regress support for other users without third-party customizations.

log.sh should have explicit error case, always is better suited to use the * fallback. Not sure why error is set as default LOG_LEVEL case to rely on * as fallback/highlighting?

Possible bug with one of your cron heredoc additions.

All resolved :D

Feedback: Personal opinion - Not a fan of HEREDOC

Cron jobs to heredocs (relevant commit with commit message for context) is not something I'm fond of personally.

I prefer external files instead of embedded/inlined, ENV as a convenience isn't sufficient for me. We already write ENV to a file for other scripts to read and utilize. I'd rather cron jobs we add are all in a common location from a maintenance perspective, but I understand if the majority of maintainers prefer the heredoc approach instead and have no interest in fighting that decision.

I have to admit I disagree, obviously (otherwise I wouldn't have done it). I first tried your approach to see how it goes, but then I noticed that it is way more work to handle these scripts if they are not "inlined" into setup-stack.sh (with a HEREDOC). Moreover, we only actually have two cron scripts that are added. From experience with this project and writing many many lines of code, I would strongly advise against dedicated scripts.

Feedback: Log changes could have been covered in a separate PR

I agree. Therefore, I opened #2486.

target/scripts/helpers/error.sh

target/scripts/startup/setup-stack.sh

…ussion_r827125866

target/scripts/helpers/error.sh

.github/ISSUE_TEMPLATE/bug_report.yml

casperklein · 2022-03-15T23:21:39Z

Feedback: Personal opinion - Not a fan of HEREDOC

Depends on the use-case IMO. Regarding the usage for our cron jobs, I think this is a good solution.

polarathene

Possibly some changes you still want to make, otherwise LGTM 👍

target/scripts/startup/setup-stack.sh

test/mail_ssl_manual.bats

target/scripts/startup/setup-stack.sh

georglauterbach · 2022-03-16T09:38:52Z

Thanks for the good feedback, very much appreciated!

target/scripts/startup/setup-stack.sh

Co-authored-by: Casper <casperklein@users.noreply.github.com>

georglauterbach added 7 commits March 14, 2022 14:25

improved the SA KAM cron job

ed2d925

The job could fail if no new fresh updates are available. This is however not a reason to receive a new email telling me the job failed. This commit provides a solution.

improved SA KAM handling

59f1a1f

After reading the docs on the exit code, we can do better.

fixed linting errors and small style adjustments

1714799

refactored linting script

7740ad4

We need not write up to loggers, sourcing the already present one is fine. This simplifies the whole script.

georglauterbach added priority/high area/scripts kind/improvement Improve an existing feature, configuration file or the documentation kind/update Update an existing feature, configuration file or the documentation labels Mar 14, 2022

georglauterbach added this to the v11.0.0 milestone Mar 14, 2022

georglauterbach requested review from casperklein, polarathene, NorseGaud and wernerfred March 14, 2022 20:24

georglauterbach self-assigned this Mar 14, 2022

georglauterbach added 2 commits March 14, 2022 21:29

better docs about LOG_LEVEL default

8be97f4

corrected lint.sh

75531fd

polarathene requested changes Mar 15, 2022

View reviewed changes

georglauterbach mentioned this pull request Mar 15, 2022

scripts: update log #2486

Closed

8 tasks

georglauterbach added 6 commits March 15, 2022 13:39

Revert "remove setup_ssl call from changedetector again"

653e76a

This reverts commit d5c7f21.

Revert "reworked log completely"

5d75d6b

This reverts commit abf18e5.

more corrections

08bd4ca

Revert 'refactored linting script'

fc81ff7

Merge remote-tracking branch 'origin/master' into fix/small-misc-scri…

da8629e

…pt-enhancements

more corrections from suggestions

3b8050a

georglauterbach changed the title ~~scripts: log update and script changes~~ scripts: small refactorings Mar 15, 2022

more correction from PR suggestions by @polarathene

6dc28be

Merge remote-tracking branch 'origin/master' into fix/small-misc-scri…

ea15fa2

…pt-enhancements

georglauterbach requested a review from polarathene March 15, 2022 13:19

adjust workflow again (revert log level additions from #2486)

0ee258f

georglauterbach removed the priority/high label Mar 15, 2022

fixed regression introduces by longer wait time in check-for-changes.sh

526ad41

casperklein reviewed Mar 15, 2022

View reviewed changes

target/scripts/helpers/error.sh Outdated Show resolved Hide resolved

target/scripts/helpers/error.sh Outdated Show resolved Hide resolved

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved

georglauterbach added 2 commits March 15, 2022 17:12

simplified SA KAM

a811061

https://github.com/docker-mailserver/docker-mailserver/pull/2485#disc…

3bd0c04

…ussion_r827125866

polarathene requested changes Mar 15, 2022

View reviewed changes

target/scripts/helpers/error.sh Outdated Show resolved Hide resolved

target/scripts/helpers/error.sh Outdated Show resolved Hide resolved

.github/ISSUE_TEMPLATE/bug_report.yml Outdated Show resolved Hide resolved

remove more leftover residue

3c74831

georglauterbach requested review from polarathene and casperklein March 16, 2022 09:16

polarathene previously approved these changes Mar 16, 2022

View reviewed changes

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved

test/mail_ssl_manual.bats Show resolved Hide resolved

target/scripts/startup/setup-stack.sh Show resolved Hide resolved

apply more feedback

d2c9aed

georglauterbach dismissed polarathene’s stale review via d2c9aed March 16, 2022 09:38

georglauterbach requested a review from polarathene March 16, 2022 09:38

polarathene previously approved these changes Mar 16, 2022

View reviewed changes

casperklein reviewed Mar 16, 2022

View reviewed changes

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved

target/scripts/startup/setup-stack.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

3a32ef1

Co-authored-by: Casper <casperklein@users.noreply.github.com>

georglauterbach dismissed polarathene’s stale review via 3a32ef1 March 17, 2022 08:04

georglauterbach requested review from casperklein and polarathene March 17, 2022 08:05

polarathene approved these changes Mar 17, 2022

View reviewed changes

casperklein approved these changes Mar 17, 2022

View reviewed changes

georglauterbach merged commit 321ae74 into master Mar 17, 2022

georglauterbach deleted the fix/small-misc-script-enhancements branch March 17, 2022 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: small refactorings #2485

scripts: small refactorings #2485

georglauterbach commented Mar 14, 2022 •

edited

github-actions bot commented Mar 14, 2022

polarathene left a comment •

edited

georglauterbach commented Mar 15, 2022

Feedback: Personal opinion - Not a fan of HEREDOC

Feedback: Log changes could have been covered in a separate PR

casperklein commented Mar 15, 2022

polarathene left a comment

georglauterbach commented Mar 16, 2022

scripts: small refactorings #2485

scripts: small refactorings #2485

Conversation

georglauterbach commented Mar 14, 2022 • edited

Description

Type of change

Checklist:

github-actions bot commented Mar 14, 2022

polarathene left a comment • edited

Choose a reason for hiding this comment

Feedback: Personal opinion - Not a fan of HEREDOC

Feedback: Log changes could have been covered in a separate PR

georglauterbach commented Mar 15, 2022

Feedback: Personal opinion - Not a fan of HEREDOC

Feedback: Log changes could have been covered in a separate PR

casperklein commented Mar 15, 2022

polarathene left a comment

Choose a reason for hiding this comment

georglauterbach commented Mar 16, 2022

georglauterbach commented Mar 14, 2022 •

edited

polarathene left a comment •

edited