Skip to content

Improve VIRUSMAILS_DELETE_DELAY usage#2281

Merged
casperklein merged 23 commits intodocker-mailserver:masterfrom
casperklein:virus-wiper
Nov 1, 2021
Merged

Improve VIRUSMAILS_DELETE_DELAY usage#2281
casperklein merged 23 commits intodocker-mailserver:masterfrom
casperklein:virus-wiper

Conversation

@casperklein
Copy link
Copy Markdown
Member

Description

The VIRUSMAILS_DELETE_DELAY variable is set in multiple places. With the introduction of /etc/dms-settings, this can be simplified.

Discussed/Discovered partly here: https://github.com/docker-mailserver/docker-mailserver/pull/2264#issuecomment-955603197#

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 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

@casperklein casperklein self-assigned this Nov 1, 2021
@casperklein casperklein added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Nov 1, 2021
@casperklein casperklein added this to the v10.3.0 milestone Nov 1, 2021
@casperklein
Copy link
Copy Markdown
Member Author

Does anyone knows, why shellcheck doesn't care about sourcing here:

. /etc/dms-settings 2> /dev/null

but complaining here?

@casperklein casperklein marked this pull request as ready for review November 1, 2021 15:36
@casperklein casperklein requested a review from a team November 1, 2021 15:36
@georglauterbach
Copy link
Copy Markdown
Member

Does anyone knows, why shellcheck doesn't care about sourcing here:

. /etc/dms-settings 2> /dev/null

but complaining here?

A very good question, I have no idea :D

@casperklein casperklein merged commit b117cd1 into docker-mailserver:master Nov 1, 2021
@casperklein casperklein deleted the virus-wiper branch November 1, 2021 21:13
@polarathene
Copy link
Copy Markdown
Member

polarathene commented Nov 1, 2021

Does anyone knows, why shellcheck doesn't care about sourcing here:

It seems this was missed during review:

--source-path=SCRIPTDIR

--source-path="${SCRIPT_DIR}" is probably what was intended as that's defined earlier in the script. No reference of SCRIPTDIR anywhere.

What I do find odd though is why the errors are reduced with that, but substituting the value with say x instead did not have the same effect but should be just as invalid 😕


EDIT: Nope, nevermind SCRIPTDIR is a special value for shellcheck (Might be worth documenting that link in the lint.sh file actually 😅 ). Without that you will get the failure for listmailuser as well among others such as the last var in start-mailserver.sh VARS list/section.

Which might hint that it's a similar behaviour with . /etc/dms-settings, it only reports the last occurrence for whatever reason.. In my common.bash PR, you'll see that the bounced spam test update adds a shellcheck disable comment only to the 2nd test case, without needing to do it for the first one despite no difference apart from declaration/source order in the file.

@casperklein
Copy link
Copy Markdown
Member Author

among others such as the last var in start-mailserver.sh VARS list/section.

I was wondering about this also some time ago 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/scripts kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants