tests(fix): Avoid introducing CRLF into postfix-accounts.cf during setup#2820
Conversation
…setup Currently a change detection would be triggered and during processing, a CRLF is converted to LF, which updates the `postfix-accounts.cf` file and triggers another change event. No need for the first approach to add an account, and it is the culprit for causing the CRLF to appear.
casperklein
left a comment
casperklein
left a comment
There was a problem hiding this comment.
Currently when a change detection would be triggered and during processing, a CRLF is found and converted to a LF, which modifies the postfix-accounts.cf file, triggering another change event 😬
Very nice investigation 👍
I found out, that the
Proof: Same, but with We see, that there is an additional carriage return |
Very interesting, thanks for sharing that finding with |
|
In my test branch, tests did immediately run successfully, after removing The solution in this PR is much clearer, than just removing I don't understand, why this was used in the first place: docker run ... > file-on-host
docker exec ... # modify file directly in the containerWhy were there two methods used for adding accounts to a running container 🤷 |
|
Hooray :D PS: Notices that CI does not use cache here as well. GH had some recent downtime, maybe there now is something wrong with our cache? I will have a look. The changes themselves should definitely not have anything to do with the current caching issue. The issue itself is minor as well, as we only loose a minute, but annoying nevertheless. |
It is using the same cache-key as I only made a change to a test which doesn't warrant caching a new image. I assume something didn't cache well with that key, but once we have a new one we'll have a better idea about cache working properly or not. |
Since it was for |
3 participants
Description
test/tests.batshas been extremely unreliable during the setup stage. This should finally resolve that. Big thanks to @casperklein and @georglauterbach who have helped rule out other causes to narrow it down to this.Recent changes I introduced to
tests.batsbetter surfaced change detection problems more consistently, to the point failure frequency was extremely probable. I investigated and identified the cause 🎉Currently when a change detection would be triggered and during processing, a
CRLFis found and converted to aLF, which modifies thepostfix-accounts.cffile, triggering another change event 😬Marked for
v11.2.0as it's only a change to our tests and should make existing PRs much happier :)Solution: Avoid introducing
CRLFinto config by adding accounts only viasetup email addThere is no need for the first approach used in
tests.batsto add an account, and it is the culprit for causing the CRLF to appear.It is unclear why a
CRLFis added to the end of the file, only when usingdocker runwith the DMS docker image to output apostfix-accounts.cfconfig line viaechoand using that stdout result to append (>>) to thepostfix-accounts.cf.It did not seem to occur via
docker execand performing a similar command within the container to update the file internally, nor without docker by updatingpostfix-accounts.cfin the same manner directly to the file at the volume mount location.Additional Details
Potential run-time modification of configs intended for fixing to keep in mind
In future, the following could be considered for removal, which may require better handling of the scripts (or ideally, porting to a more capable language where this type of problem would not cause bugs when processing configs):
docker-mailserver/target/scripts/helpers/accounts.sh
Line 25 in ff96950
docker-mailserver/target/scripts/helpers/accounts.sh
Line 32 in ff96950
docker-mailserver/target/scripts/helpers/accounts.sh
Line 181 in ff96950
docker-mailserver/target/scripts/helpers/accounts.sh
Line 187 in ff96950
docker-mailserver/target/scripts/helpers/aliases.sh
Line 20 in ff96950
I do not have the time to ensure removing those modifications would not risk causing problems with our scripts that read them. We also have another issue open regarding making a copy of config files to read to avoid issues with something else modifying the file while we process it.
History of these particular lines
sed -i -e '$a\' /tmp/postfix/accounts.cfintroduced in Aug 2015 by original DMS project author. No PR, but does reference an issue the commit is meant to resolve - however I don't see anything in the discussion that looks related to it being added. I assume it's mostly specific to compatibility with shell scripts and unix utilities that expect it?sed -i 's/\r//g' /tmp/docker-mailserver/postfix-accounts.cfintroduced April 2016, which references an issue where a user experienced a bug from theirpostfix-accounts.cfhaving aCRLFpresent, scripts did not generate accounts in Dovecot/etc/dovecot/userdbproperly.sed -i -e "s|, |,|g" -e "s|,$||g" "${DATABASE_VIRTUAL}"introduced in March 2018 to better support alias management (notablysetup alias del) so that,would be normalized to always having a space suffix,.Technically, if config files are only managed by our
setupcommand, then there should be no need for any of these I think? Users may manually edit the files directly or have setup some other way to modify / create the configs, like the legacymake generate-accountsapproach and what we see here intests.bats.Thus these are more a precaution / safe-guard. If we don't care about external config updates being made while the container is running, and not having change events "repair" / fix those potential issues, then we could move these into startup or
setupfixes, or into the docs.Otherwise they're not likely to cause much trouble, they were just silently messing with this particular test file for some time. While
make generate-accountsprobably introduces CRLF as well, those were handled during startup before a checksum was made for the change detector, thus already fixed ahead of time.Type of change
Checklist: