WIP: Refactord all scripts, adding shellcheck to travis; refactoring a few other files on the fly by georglauterbach · Pull Request #1599 · docker-mailserver/docker-mailserver

georglauterbach · 2020-09-04T15:50:31Z

This is a bigger one

This was a lot of work. Please consider this before arguing. I know it's kind of big, but I think it's worth the hassle.

First things first

This PR obsoletes Add ShellCheck tests and recommendations #1228 and Fix inconsistent indentation #1550. Please close these issues.
This PR is based on both major refactoring for setup.sh (#1590) #1595 and Populate relayhost_map from virtual accounts (and other fixes) #1596. They will need to be merged first. (major refactoring for setup.sh (#1590) #1595 is already.)

Help needed

The current problem I'm working on arises from a (locally executed) test:

 ✗ checking spoofing: accepts sending as alias
   (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
    in test file test/tests.bats, line 1503)
     `assert_success' failed

   -- command failed --
   status : 1
   output :
   --

This is kind of ridiculous:

not ok 2 check helper function: _sanitize_ipv4_to_subnet_cidr
 (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
  in test file test/helper_functions.bats, line 9)
   `assert_output "0.0.0.0/0"' failed
 ./target/helper_functions.sh: line 39: mapfile: -d: invalid option
 mapfile: usage: mapfile [-n count] [-O origin] [-s count] [-t] [-u fd] [-C callback] [-c quantum] [array]
 ./target/helper_functions.sh: line 29: & 0: syntax error: operand expected (error token is "& 0")
 ./target/helper_functions.sh: line 29: & 0: syntax error: operand expected (error token is "& 0")
 ./target/helper_functions.sh: line 29: & 0: syntax error: operand expected (error token is "& 0")
 ./target/helper_functions.sh: line 29: & 0: syntax error: operand expected (error token is "& 0")
 
 -- output differs --
 expected : 0.0.0.0/0
 actual   : .../0
 --

Using this function locally works perfect. The use of mapfile -d '.' ... in other places does not seem to throw an error. This seems to be related with the version of Bash used**?** I'm on Bash 5.0.17.

I would like to receive some help here.

This is rather interesting: I do not get this error locally too.

not ok 260 checking setup.sh: setup.sh email add and login
 (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
  in test file test/tests.bats, line 1251)
   `assert_output "passdb: [email protected] auth succeeded"' failed
 
 -- output differs --
 expected : passdb: [email protected] auth succeeded
 actual   : passdb: [email protected] auth failed
 --

Changes & Rationale

I made this PR to include shellcheck and to make all the scripts more robust. I believe the latter to be especially important. Therefore, I refactored all scripts under target/ (which was a lot of work). I furthermore extended CONTRIBUTING.md with necessary coding style guidelines, and linking to it from README. This accounts for the biggest portion of this PR.

Travis should now use the shellcheck option and apply it via make.

I refactored the Makefile too, to make it verbose where it should be, and less verbose where it's appropriate.

Important

I'd really like to see some contributions in the form of either objective opinions or (better :D) code contributions to fix possible errors.

major refactoring for setup.sh (#1590)

`set -u` stopped dkim generation from defaulting

changed to bash and implemented simple error logging

fixed $VOLUME not being set and refactored README as well due to markdownlint showing (valid) complaints

If a change to one of the tracked files happened soon after (<1 second?) a previously detected change, it could end up going undetected. In particular, this could cause integration tests to fail (see next commits). Fixed by computing the new checksum file _before_ checking for changes.

check-for-changes.sh did not have a special case to handle lines in postfix-relaymap.cf consisting of only a domain (indicating that said domain should never be relayed). This case is handled by start-mailserver.sh so when such a line existed, things would work well until a config file update was detected by check-for-changes.sh. After that, the generated relayhost_map file would be corrupted. Fixed by factoring a 'populate_relayhost_map' function out of start-mailserver.sh and into helper_functions.sh and reusing it in check-for-changes.sh. Note: There are certainly quite a few more pieces of code that could be refactored in a similar fashion. Note2: check-for-changes.sh would previously never update the relayhost_map file when $ENABLE_LDAP was set to 1. I don't think this was intended —there is after all no such condition in start-mailserver.sh— and so this condition no longer applies.

Previously, only postfix-relaymap.cf and postfix-accounts.cf would be used to populate the relayhost_map file. Now, also use postfix-virtual.cf when present. To me, there is nothing absurd about sending mail "From:" a virtual account (or more specifically its domain) so it makes sense that when a $RELAY_HOST is defined it should be used for virtual accounts as well.

setup.sh more consistent with braces, return codes, un-setting of variables and error reports and (importantly) test

checked and streamlined fail2ban and postfix wrapper

Relayhostmap from virtual

…d panic

erik-wramner · 2020-09-05T11:51:22Z

I really appreciate all the work you have put into this and it is absolutely worthwhile. However, based on previous similar attempts that have lost speed after a while and our limited review capacity I would suggest splitting the massive PR into many small parts, where we can merge and get the benefits from one change without having to wait for all of them to go through.

That is just a suggestion. I'll get around to this eventually regardless, but I'm short on time as always and it tends to be easier to merge many small changes on at a time.

georglauterbach · 2020-09-05T11:59:01Z

@erik-wramner You're right. I came up with this: All changes not related to start-mailserver.sh will be thrown into another PR. Those account for the minority of changes. The changes to start-mailserver.sh will receive their own PR. I will need #1596 for this to be merged though. How fast can you arrage this?

georglauterbach · 2020-09-05T12:00:52Z

PS: I read about the limited review capacity. I'd like to help there, but I'd need to be a collaborator for this (or don't I?). What can we do abut this?

erik-wramner · 2020-09-05T12:04:52Z

It is merged now. You would need access from @tomav to actually do merges, but you're welcome to help with reviews of other PRs if you like. Not to mention answering questions, that is where I spend the bulk of my time.

georglauterbach · 2020-09-05T12:08:04Z

How would I go about contacting @tomav, @erik-wramner ?

My rationale is this: I'm using the server and wan't it to be stable. Therefore I'd like to collaborate, answer questions and review/manager PRs :)

georglauterbach · 2020-09-05T13:32:34Z

Closing this PR as #1601 will come up soon.

tomav · 2020-09-07T11:25:12Z

@aendeavor thanks for this very good work and welcome onboard!

Georg Lauterbach and others added 20 commits August 24, 2020 11:00

f8fc0e7

major refactoring for setup.sh (#1590)

fc5f316

`set -u` stopped dkim generation from defaulting

d082c4f

changed to bash and implemented simple error logging

f792fc4

fixed $VOLUME not being set and refactored README as well due to markdownlint showing (valid) complaints

a56ab8c

setup.sh more consistent with braces, return codes, un-setting of variables and error reports and (importantly) test

b0cbbf7

checked and streamlined fail2ban and postfix wrapper

Merge pull request #2 from mwnx/relayhostmap-from-virtual

d1c6f0e

Relayhostmap from virtual

added shellcheck to Travis; extended contributing; adjusted wrapper

e205e12

streamlined check-for-changes.sh; started start-mailserver.sh

1b9c7d4

finished start-mailserver; minor fixes in other scripts; adjusted travis

0e113b7

cleaned up Makefile; fixed vhosts error; implemented shellcheck test

505a1d2

adjusted CONTRIBUTING and README; minor corrections in start-mailserver

051001a

added strip_ansi=true to fix an error where supervisord tail woul…

e6b3c28

…d panic

Merge branch 'master' into shellcheck

8af6e53

fixing TravisCI's use of shellcheck

e8ac833

corrected shellcheck tests

6737bed

adjusted for Bash4's bitwise AND-ing for TravisCI

65a50c9

georglauterbach changed the title ~~WIP / HELP WANTED: Refactord all scripts, adding shellcheck to travis; refactoring a few other file on the fly~~ WIP / HELP WANTED: Refactord all scripts, adding shellcheck to travis; refactoring a few other files on the fly Sep 4, 2020

georglauterbach marked this pull request as draft September 5, 2020 11:56

georglauterbach closed this Sep 5, 2020

georglauterbach mentioned this pull request Sep 5, 2020

Changes from #1599 without start-mailserver.sh #1601

Merged

1 task

georglauterbach changed the title ~~WIP / HELP WANTED: Refactord all scripts, adding shellcheck to travis; refactoring a few other files on the fly~~ WIP: Refactord all scripts, adding shellcheck to travis; refactoring a few other files on the fly Sep 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP: Refactord all scripts, adding shellcheck to travis; refactoring a few other files on the fly#1599

WIP: Refactord all scripts, adding shellcheck to travis; refactoring a few other files on the fly#1599
georglauterbach wants to merge 20 commits intodocker-mailserver:masterfrom
georglauterbach:shellcheck

georglauterbach commented Sep 4, 2020 •

edited

Loading

Uh oh!

erik-wramner commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020

Uh oh!

erik-wramner commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020 •

edited

Loading

Uh oh!

tomav commented Sep 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

georglauterbach commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This is a bigger one

First things first

Help needed

Changes & Rationale

Important

Uh oh!

erik-wramner commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020

Uh oh!

erik-wramner commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020

Uh oh!

georglauterbach commented Sep 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomav commented Sep 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

georglauterbach commented Sep 4, 2020 •

edited

Loading

georglauterbach commented Sep 5, 2020 •

edited

Loading