New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
style: minor adjustments #2786
style: minor adjustments #2786
Conversation
These adjustments are jus follow-ups for the latest PRs I merged.
@@ -130,7 +130,7 @@ function _install_fail2ban | |||
curl -Lkso fail2ban.deb "${FAIL2BAN_DEB_URL}" | |||
curl -Lkso fail2ban.deb.asc "${FAIL2BAN_DEB_ASC_URL}" | |||
|
|||
FINGERPRINT=$(LANG=C gpg --verify fail2ban.deb.asc fail2ban.deb 2>&1 | sed -n 's#Primary key fingerprint: \(.*\)#\1#p') | |||
FINGERPRINT=$(LANG=C gpg --verify fail2ban.deb.asc fail2ban.deb |& sed -n 's#Primary key fingerprint: \(.*\)#\1#p') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't expect |&
to have much luck with search engines, but I stand corrected 😅 (I did see the helpful commit message prior to searching, but was curious)
I suppose we have a lot of bash specific syntax throughout the project that a shell like ash
probably isn't compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly, how compatible ash/dash etc. are to bash. But as we wrote our scripts for bash, I am very fine with using bash specific syntax to unleash the full power 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@docker-mailserver/maintainers While we at it: what about replacing the sed
command with:
grep -oP '(?<=Primary key fingerprint: ).+'
Regex explanation: The ?<=
within the group means --> ignore this group and match only .+
that comes after.
This seems simpler and easier read (for me at least).
But no need for a lengthy discussion IMO. Just give a 👎 to keep it as it is, or 👍 to go with grep
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's worth it personally.
Very minor improvement, and not something I'm going to remember, or like looking up if I saw it months later and forgot (git blame
aside).
The sed
command is familiar enough, I've seen the usage here elsewhere in the project, so it's easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see an inherent need for this change as well - doesn't really matter I think. I'd just like to merge this sometime soon so we can get a feature freeze in place for v11.2.0 :)
@casperklein currently tests failed with the healthcheck, which I think you tried to address with a recent PR:
|
We will definitely need to do something about the issue where only 217 tests run. The |
When you both experienced the test 217 failures, I assume it was on PRs that would not result in invalidating the ClamAV cache, and that cache may be old enough to have ClamAV update when the container runs? If so I have noticed on my system how problematic that can be for getting |
Yes, this is absolutely possible.
Interesting - I will have a look into this too. Will provide more information in #2811. |
Description
These adjustments are just follow-ups for the latest PRs I merged. If you have more adjustments, feel free to add them to this PR.
Type of change
Checklist:
docs/
)