scripts: refactored scripts located under `target/bin/` by georglauterbach · Pull Request #2500 · docker-mailserver/docker-mailserver

georglauterbach · 2022-03-21T10:27:45Z

Description

The scripts under target/bin/ now use the new log and I replaced some
"" with '' on the way. The functionality stays the same, this mostly
style and log.

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

The scripts under `target/bin/` now use the new log and I replaced some `""` with `''` on the way. The functionality stays the same, this mostly style and log.

…/target-bin

Moreover, a few messages for BATS were streamlined and a regression in the linting script reverted.

The new output has a single, clear message with the '[ ERROR ] ' prefix, and then output that explains the error afterwards. This is coherent with the logging style which should be used while providing more information than just a single line about IPTables not functioning.

Before, scripts located under `target/bin/` were using `usage` or `__usage`. Now, they're using `__usage` as they should.

georglauterbach · 2022-03-22T10:26:44Z

It seems I was (literally less than) 1 minute too late. I noticed that usage was used inconsistently @polarathene - sorry for getting on your nerves with reviewing this (again 😆)... I promise this was my final commit :D

polarathene

LGTM 👍

With `sedfile`, we cannot use the helper functions in a nice way because it is used early in the Dockerfile at a stage where the helper scripts are not yet copied. The script has been adjusted to be canonical with all the other scripts under `target/bin/`.

georglauterbach · 2022-03-23T10:03:45Z

I have added another commit (4560650) to adjust sedfile (due to feedback by @casperklein) to be canonical with the other scripts under target/bin/. I think further changes are not required.

georglauterbach · 2022-03-23T11:01:01Z

🤦🏼 🤦🏼 🤦🏼 Nice test failure (due to not matching substring). @polarathene sorry for updating the PR again. I feel like I'm pestering you, and I promise, I'm not doing it on purpose :)

`__usage` is to be used on wrong user input, not on other failures as well. This was fixed in `delquota` and `setquota`.

casperklein · 2022-03-24T23:11:12Z

 set -ueo pipefail

-HASHTOOL="sha1sum"
+function __usage { echo "Usage: ${0} -i <replace/delete operation> <file>" ; }


Why introducing a function, that is only called once?

Probably the intention was to just be consistent with other CLI utility scripts in this folder? That's not such a bad thing even if it might be redundant in this particular file 😅

All the changes are done for uniformity with the other scripts. In #2493 (comment) you pledged for a "continuous look and feel" - this is it (looking at all the other scripts). I have become a bit tired of never-ending small nitpicks like this to be frank. Therefore, I will leave this in (because it really is a nice addition to script uniformity), but I will revert some of the other changes to sedfile - otherwise I fear, these changes might never end up in master...

you pledged for a "continuous look and feel" - this is it

Fair point. And with that argumentation, I agree with keeping _usage. For the same reason I wonder, why you decline to introduce _log in this PR? Technically it's no problem to copy the needed files before the first usage of sedfile.
Keeping the echos is clearly against a "continuous look and feel".

If you want to add _log to sedfile do it as a follow-up PR, lets just get this stuff merged while it's in a good state and approved 😅

I agree with @polarathene :) @casperklein I will add a smaller follow-up PR that introduces _log to sedfile :D

…into scripts/refactoring/target-bin

refactored scripts located under target/bin/

77e7d63

The scripts under `target/bin/` now use the new log and I replaced some `""` with `''` on the way. The functionality stays the same, this mostly style and log.

georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Mar 21, 2022

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

georglauterbach requested review from casperklein and polarathene March 21, 2022 10:27

georglauterbach self-assigned this Mar 21, 2022

georglauterbach marked this pull request as draft March 21, 2022 12:03

georglauterbach added 4 commits March 21, 2022 19:22

Merge remote-tracking branch 'origin/master' into scripts/refactoring…

55c9be1

…/target-bin

corrected fail2ban (script and tests)

5e2be42

corrected OpenDKIM log output in tests

ddb09f8

reverted (some) changes to sedfile

8cc79fc

Moreover, a few messages for BATS were streamlined and a regression in the linting script reverted.

georglauterbach marked this pull request as ready for review March 21, 2022 21:07

casperklein reviewed Mar 21, 2022

View reviewed changes

Comment thread target/bin/sedfile Outdated

casperklein reviewed Mar 21, 2022

View reviewed changes

Comment thread target/bin/sedfile Outdated

polarathene requested changes Mar 22, 2022

View reviewed changes

apple PR feedback

08a05de

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

This comment was marked as resolved.

Sign in to view

polarathene previously approved these changes Mar 22, 2022

View reviewed changes

Comment thread target/bin/setquota

simplified setquota script

9d3eda4

georglauterbach dismissed polarathene’s stale review via 9d3eda4 March 22, 2022 10:17

polarathene previously approved these changes Mar 22, 2022

View reviewed changes

consistently named the __usage function

1ce31df

Before, scripts located under `target/bin/` were using `usage` or `__usage`. Now, they're using `__usage` as they should.

docker-mailserver deleted a comment from georglauterbach Mar 22, 2022

georglauterbach dismissed polarathene’s stale review via 1ce31df March 22, 2022 10:25

georglauterbach requested a review from polarathene March 22, 2022 10:27

polarathene previously approved these changes Mar 22, 2022

View reviewed changes

casperklein requested changes Mar 23, 2022

View reviewed changes

Comment thread target/bin/sedfile Outdated

Comment thread target/bin/sedfile Outdated

Comment thread target/bin/sedfile Outdated

Comment thread target/bin/setquota

Comment thread target/bin/updatemailuser

improved sedfile

4560650

With `sedfile`, we cannot use the helper functions in a nice way because it is used early in the Dockerfile at a stage where the helper scripts are not yet copied. The script has been adjusted to be canonical with all the other scripts under `target/bin/`.

georglauterbach dismissed polarathene’s stale review via 4560650 March 23, 2022 09:58

georglauterbach requested review from casperklein and polarathene March 23, 2022 10:04

polarathene previously approved these changes Mar 23, 2022

View reviewed changes

fixed tests

f2fe2ad

georglauterbach dismissed polarathene’s stale review via f2fe2ad March 23, 2022 10:59

polarathene previously approved these changes Mar 23, 2022

View reviewed changes

casperklein reviewed Mar 23, 2022

View reviewed changes

Comment thread target/bin/setquota Outdated

removed __usage from places where it does not belong

30ccbb7

`__usage` is to be used on wrong user input, not on other failures as well. This was fixed in `delquota` and `setquota`.

georglauterbach dismissed polarathene’s stale review via 30ccbb7 March 24, 2022 09:08

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

Merge branch 'master' into scripts/refactoring/target-bin

56780c4

polarathene previously approved these changes Mar 24, 2022

View reviewed changes

casperklein reviewed Mar 24, 2022

View reviewed changes

georglauterbach added 2 commits March 25, 2022 09:39

apply PR review feedback

ed512d0

Merge remote-tracking branch 'origin/scripts/refactoring/target-bin' …

a521a78

…into scripts/refactoring/target-bin

georglauterbach dismissed polarathene’s stale review via a521a78 March 25, 2022 08:40

georglauterbach requested a review from casperklein March 25, 2022 08:47

polarathene approved these changes Mar 25, 2022

View reviewed changes

casperklein approved these changes Mar 25, 2022

View reviewed changes

georglauterbach merged commit b9dbec3 into master Mar 26, 2022

georglauterbach deleted the scripts/refactoring/target-bin branch March 26, 2022 08:30

georglauterbach mentioned this pull request Mar 26, 2022

scripts: introduce _log to sedfile #2507

Merged

7 tasks

Uh oh!

Conversation

georglauterbach commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

georglauterbach commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georglauterbach commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

georglauterbach commented Mar 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

casperklein Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

polarathene Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

georglauterbach Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

casperklein Mar 25, 2022

Choose a reason for hiding this comment

Uh oh!

polarathene Mar 26, 2022

Choose a reason for hiding this comment

Uh oh!

georglauterbach Mar 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

georglauterbach commented Mar 21, 2022 •

edited

Loading

georglauterbach commented Mar 22, 2022 •

edited

Loading

georglauterbach commented Mar 23, 2022 •

edited

Loading

georglauterbach Mar 25, 2022 •

edited

Loading