MacOS linting & testing support + docs #2001

NorseGaud · 2021-05-24T13:14:40Z

Description

macOS users cannot easily contribute as linting and testing doesn't work locally. Some code is linux/GNU only and breaks when running on mac for various reasons.

A key change: Linting no longer requires you to run things on your host.

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

georglauterbach · 2021-05-25T16:49:30Z

Since this builds on #2000, it does not need to incorporate the same changes again. When #2000 is merged, this PR will possibly have unnecessary merge conflicts. Please remove the changes that are already present in #2000.

Not sure about #1980, but it seems to have these very changes too if I‘m not mistaken.

polarathene

My review is mostly regarding the docs, but I have a few concerns with other changes that I'd appreciate some clarity on.

docs/content/contributing/tests.md

Makefile

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

polarathene

Looks like we just have the switch from the current docker image approach for linting to docker-compose using official lint images directly and we're good! 🎉

For the other maintainers, note that the improvement to image name collection was reverted. It did reveal that our current filter likewise isn't identifying all test images by name assigned, mostly because there is no established/documented convention enforced which should be addressed at some point.

docs/content/contributing/tests.md

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

setup.sh

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

github-actions · 2021-06-07T10:59:43Z

Documentation preview for this PR is ready! 🎉

Built with commit: 7707464

polarathene

Awesome work @NorseGaud ! Thanks for taking the time to contribute 😀

TL;DR for reviewers of PR thread thus far

Summary of anything possibly worth pointing out:

#2001 (comment) notes that this should have been reverted, but has resulted in: "${@:+$@}", apparently due to "unbound variable problems".

#2001 (comment) notes that the change from find option -executable for compatibility reasons in lint.sh may result in potential false positives, but it's probably unlikely for this project:

[[ "$(uname)" == "Darwin" ]] && PERM="+111" || PERM="/111" 
F_BIN="$(find 'target/bin' -perm "${PERM}" -type f -or -type l)"

#2001 (comment) notes another minor find change with test instead of test/ for matching the target directory to scan (this only covers a limited depth mind you, not sure if that's intentional to skip some of the contents further down?). Should otherwise be harmless?

#2001 (comment) notes that logging the linter version has been dropped. If maintainers still desire that, now that we use docker images for linting, the versions in the variables should be sufficient to log with?

#2001 (comment) notes ERR has been dropped in lint.sh, but is still referenced by this line:

trap '__log_err "${FUNCNAME[0]:-?}" "${BASH_COMMAND:-?}" ${LINENO:-?} ${?:-?}' ERR

#2001 (review) notes unrelated to changes in this PR, our cleanup logic in Makefile needs to be addressed, tests aren't conforming to conventions preventing them from being matched.

#2001 (comment) notes unrelated to this PR, the issues and pull requests docs page still has a rendering issue of nested lists.

polarathene · 2021-06-07T22:52:26Z

@aendeavor is the trap line with ERR at the end still valid?

georglauterbach · 2021-06-08T08:42:23Z

@aendeavor is the trap line with ERR at the end still valid?

Yes :) In case of an unexpected error, it will print information via the __log_err function. This can help debugging or with issue reports. I'd leave it in there.

Makefile

casperklein · 2021-06-08T14:32:16Z

setup.sh

@@ -225,7 +231,7 @@ function _docker_image
  if ${USE_CONTAINER}
  then
    # reuse existing container specified on command line
-    ${CRI} exec "${USE_TTY}" "${CONTAINER_NAME}" "${@}"
+    ${CRI} exec "${USE_TTY}" "${CONTAINER_NAME}" "${@:+$@}"


I don't understand the purpose of all these "${@:+$@}".

If $@ is not empty; then use $@?

@casperklein https://stackoverflow.com/questions/44567534/work-around-for-unbound-variable-in-bash-4-0-0

Did you encounter a problem or was this added in good faith?

BASH 4.0.0 was released more than 10 years ago. And the bug got fixed in later 4.x releases (just verified it with debian9 which included BASH 4.4).

Even with the latest bash in macOS it was throwing the error for me. It's a macOS specific fix

Isn't Mac using Bash 3 by default? Or did you install bash 4 via brew

Also, this all presumes the user has /usr/local/bin in their PATH before /bin... I personally think ${@:+$@} is worth it.

On my mac i needed to hash -r after brew install bash and this only works bc /usr/local/bin ist listed first on default environments. So if it is environment safe as long as bash4+ is installed anywhere on the system we should go with ${@:+$@} i think

Not sure TBH. This construct just looks weird to me 😆

But I guess we could make an effort to support macOS users:)

Isn't Mac using Bash 3 by default?

Holy shit. YMMD 😄 Couldn't believe that's true.

Is it worth leaving a comment with description or link about it for future maintainer reference? Or is the link to this PR with lengthy discussion thread to wade through sufficient?

casperklein · 2021-06-08T14:39:38Z

test/linting/lint.sh

@@ -6,6 +6,13 @@

 SCRIPT="lint.sh"

+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"


SCRIPT_DIR=$(dirname "$(readlink -f "$0")")

I'll fix in #1980

…r-mailserver/pull/2001\#discussion_r647509043

…001\#discussion_r647496925

polarathene · 2021-06-08T23:04:53Z

Yes :) In case of an unexpected error, it will print information via the __log_err function. This can help debugging or with issue reports. I'd leave it in there.

@aendeavor I meant the ERR variable part as it no longer exists anywhere else in the file?

georglauterbach · 2021-06-09T08:18:09Z

Yes :) In case of an unexpected error, it will print information via the __log_err function. This can help debugging or with issue reports. I'd leave it in there.

@aendeavor I meant the ERR variable part as it no longer exists anywhere else in the file?

I understand. Then this may not be needed anymore and can be removed entirely I guess.

…r-mailserver/pull/2001\#discussion_r647509043

…001\#discussion_r647496925

…r-mailserver/pull/2001\#discussion_r647509043

…001\#discussion_r647496925

NorseGaud mentioned this pull request May 24, 2021

chore(ci): Linting Improved #2000

Merged

7 tasks

NorseGaud changed the title ~~MacOS linting + testing support + docs~~ MacOS linting & testing support + docs May 24, 2021

wernerfred assigned NorseGaud May 24, 2021

wernerfred added the meta/feature freeze On hold due to upcoming release process label May 24, 2021

wernerfred added this to the v10.0.1 milestone May 24, 2021

NorseGaud force-pushed the macos-lint-and-test-support branch 2 times, most recently from 87740dc to b3e5d20 Compare May 24, 2021 19:25

NorseGaud added 2 commits May 25, 2021 06:53

macOS linting and testing support, safer make clean

af26ac3

Merge branch 'master' into macos-lint-and-test-support

6ebd22a

NorseGaud force-pushed the macos-lint-and-test-support branch from b3e5d20 to 6ebd22a Compare May 25, 2021 10:54

NorseGaud mentioned this pull request May 25, 2021

chore(scripts): Removing flock so NFS works #1980

Merged

11 tasks

This comment has been minimized.

Sign in to view

polarathene requested changes May 31, 2021

View reviewed changes

docs/content/contributing/tests.md Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

docs/content/contributing/issues-and-pull-requests.md Show resolved Hide resolved

wernerfred removed the meta/feature freeze On hold due to upcoming release process label Jun 1, 2021

merge master upstream

1f574d3

NorseGaud force-pushed the macos-lint-and-test-support branch from 032869c to 1f574d3 Compare June 3, 2021 01:47

NorseGaud added 2 commits June 2, 2021 22:04

doc fix to include clean again

b72da70

admonition

c351acf

NorseGaud requested a review from polarathene June 3, 2021 02:11

NorseGaud added 2 commits June 3, 2021 06:46

quick Admonition fix

8783073

--no-run-if-empty isn't supported for macOS xargs, so need to use a loop

dd660e8

NorseGaud force-pushed the macos-lint-and-test-support branch from bc73591 to dd660e8 Compare June 4, 2021 01:41

polarathene requested changes Jun 4, 2021

View reviewed changes

docs/content/contributing/tests.md Outdated Show resolved Hide resolved

Reworked linting in docker containers

cb132f2

NorseGaud force-pushed the macos-lint-and-test-support branch from 3612aa8 to cb132f2 Compare June 4, 2021 03:17

Update docs/content/contributing/tests.md

52e466a

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

georglauterbach reviewed Jun 4, 2021

View reviewed changes

setup.sh Outdated Show resolved Hide resolved

Update test/linting/lint.sh

7707464

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

polarathene approved these changes Jun 7, 2021

View reviewed changes

georglauterbach approved these changes Jun 7, 2021

View reviewed changes

georglauterbach removed request for casperklein and wernerfred June 7, 2021 12:55

georglauterbach added area/ci area/scripts kind/improvement Improve an existing feature, configuration file or the documentation kind/new feature A new feature is requested in this issue or implemeted with this PR priority/medium os/macOS labels Jun 7, 2021

georglauterbach merged commit 543bd8b into docker-mailserver:master Jun 7, 2021

georglauterbach mentioned this pull request Jun 7, 2021

docs(deps): bump mkdocs-material to 7.1.7 #2028

Merged

11 tasks

NorseGaud deleted the macos-lint-and-test-support branch June 7, 2021 21:58

casperklein reviewed Jun 8, 2021

View reviewed changes

Makefile Show resolved Hide resolved

casperklein reviewed Jun 8, 2021

View reviewed changes

NorseGaud referenced this pull request in NorseGaud/docker-mailserver Jun 8, 2021

SCRIPT_DIR to readlink -f: https://github.com/docker-mailserver/docke…

ff57990

…r-mailserver/pull/2001\#discussion_r647509043

NorseGaud referenced this pull request in NorseGaud/docker-mailserver Jun 8, 2021

fix for https://github.com/docker-mailserver/docker-mailserver/pull/2…

5084caf

…001\#discussion_r647496925

NorseGaud referenced this pull request in NorseGaud/docker-mailserver Sep 9, 2021

SCRIPT_DIR to readlink -f: https://github.com/docker-mailserver/docke…

2ece2e0

…r-mailserver/pull/2001\#discussion_r647509043

NorseGaud referenced this pull request in NorseGaud/docker-mailserver Sep 9, 2021

fix for https://github.com/docker-mailserver/docker-mailserver/pull/2…

4ec444c

…001\#discussion_r647496925

casperklein mentioned this pull request Sep 11, 2021

Move setup process via script into container #2174

Merged

8 tasks

NorseGaud referenced this pull request in NorseGaud/docker-mailserver Sep 11, 2021

SCRIPT_DIR to readlink -f: https://github.com/docker-mailserver/docke…

d703b45

…r-mailserver/pull/2001\#discussion_r647509043

NorseGaud referenced this pull request in NorseGaud/docker-mailserver Sep 11, 2021

fix for https://github.com/docker-mailserver/docker-mailserver/pull/2…

620f637

…001\#discussion_r647496925

polarathene mentioned this pull request Sep 19, 2021

tests: Refactored bounced spam test + Introduce common container setup template #2198

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MacOS linting & testing support + docs #2001

MacOS linting & testing support + docs #2001

NorseGaud commented May 24, 2021 •

edited

Loading

georglauterbach commented May 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

polarathene left a comment

polarathene left a comment

github-actions bot commented Jun 7, 2021

polarathene left a comment •

edited

Loading

polarathene commented Jun 7, 2021

georglauterbach commented Jun 8, 2021

casperklein Jun 8, 2021

NorseGaud Jun 8, 2021

casperklein Jun 8, 2021

NorseGaud Jun 8, 2021

wernerfred Jun 8, 2021 •

edited

Loading

NorseGaud Jun 9, 2021

wernerfred Jun 9, 2021

georglauterbach Jun 9, 2021

casperklein Jun 9, 2021

polarathene Jun 9, 2021

casperklein Jun 8, 2021

NorseGaud Jun 8, 2021

polarathene commented Jun 8, 2021 •

edited

Loading

georglauterbach commented Jun 9, 2021

		@@ -6,6 +6,13 @@

		SCRIPT="lint.sh"

		SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"

MacOS linting & testing support + docs #2001

MacOS linting & testing support + docs #2001

Conversation

NorseGaud commented May 24, 2021 • edited Loading

Description

Type of change

Checklist:

georglauterbach commented May 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

polarathene left a comment

Choose a reason for hiding this comment

polarathene left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 7, 2021

polarathene left a comment • edited Loading

Choose a reason for hiding this comment

TL;DR for reviewers of PR thread thus far

polarathene commented Jun 7, 2021

georglauterbach commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wernerfred Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polarathene commented Jun 8, 2021 • edited Loading

georglauterbach commented Jun 9, 2021

NorseGaud commented May 24, 2021 •

edited

Loading

polarathene left a comment •

edited

Loading

wernerfred Jun 8, 2021 •

edited

Loading

polarathene commented Jun 8, 2021 •

edited

Loading