Rspamd: script stabilization pt. 1

[georglauterbach]

Description

1. added checks whether OpenDKIM/OpenDMARC/policyd-spf/Postgrey are enabled
2. added functions to check if VAR is 0/1 or an int
3. added more checks to Rspamd setup (mainly functions introduced in 2.)
4. added canonical directory for users to place files in

Related to #3249
Closes #3266

Type of change

• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change that does improve existing functionality)
• 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]

Currently blocked by #3263, since #3263 is resolving the policyd-spf issue we see in the tests (that has nothing to do with this PR).

[polarathene]

I am presently reviewing / investigating the mydestination PR. This is requiring a deep-dive, I will review this PR next, but that could be at least a day or so minimum.

It might be wise to merge #3266 so that :edge doesn't remain broken for everyone? Then rebase / merge this PR over the updated master branch? (any concerns with that?)

[georglauterbach]

I am presently reviewing / investigating the mydestination PR. This is requiring a deep-dive, I will review this PR next, but that could be at least a day or so minimum.

I had hoped @casperklein could provide a review for this PR. I thought I just assigned a review to you @polarathene in #3257, but it seems like my figers were faster than my head.

It might be wise to merge #3266 so that :edge doesn't remain broken for everyone? Then rebase / merge this PR over the updated master branch? (any concerns with that?)

Although there is no technical issue, I'd really like to get a review in for this PR, because I have already prepared two PRs that are based on changes this PR introduces. I have a bit of time pressure because I will be starting my masters thesis soon, and this will take up most of my time then, hence I'd like to get these changes out.

I will merge #3266 now though, and rebase this PR, because fixing :edge is more important than a review for this PR.

---

UPDTAE: Rebasing done.

[polarathene]

I had hoped @casperklein could provide a review for this PR.

I believe similar to me he has low review capacity 😅

It is much easier when the scope is minimized. I know as the contributor it's annoying juggling multiple PRs, but on the reviewer end it's faster when the diff is small and focused.

I haven't looked into this one yet, so not saying that's not the case, but you have had a habit of bundling changes for convenience that could be smaller PRs (like the EOF fix).

---

Although there is no technical issue, I'd really like to get a review in for this PR, because I have already prepared two PRs that are based on changes this PR introduces.

At a glance, the PR is only touching rspamd files/docs/tests, besides the two extra helper methods you added.

I am rather relaxed on review with rspamd work while you iterate on it. Instead I've only actually noticed issues at a later point and raised them when I became aware of them.

I don't mind approving this and reviewing it after the fact since the scope only affects rspamd, there isn't as much risk impact elsewhere in the stack for me to consider 👍

---

I have a bit of time pressure because I will be starting my masters thesis soon, and this will take up most of my time then, hence I'd like to get these changes out.

I think @casperklein will share the same stance for this PR, merging should be ok and we can still provide review.

Likewise, review of any follow-up work may take some time, so hopefully it's not too much more to review. I think you mostly wanted to address some rspamd PRs and publish v12.1, and presumably get v13 out before your masters thesis starts?

If those PRs keep changes isolated to rspamd, it's easier for me to approve with a deferred review (but realistically, right now I may not manage to get around to it). If the review process is adding too much friction, we could consider bypassing review for your PRs, but I think the quality control / feedback of reviews is better than rushing changes.

I try to make time for changes that are potentially disruptive / complicated so we avoid breaking things. I would love to be more involved in the rspamd feature work, but can't afford more distractions 😅

Usual "reduced availability" note

I need to focus on clearing my backlog which will take me quite a while (and depending on circumstances may not be easy), so the less time I need to put into reviews / collaboration, the better for me atm. This project alone, I've got some fairly old WIP PR items and TODO issues that I'd like to resolve 😬

[georglauterbach]

I have a bit of time pressure because I will be starting my masters thesis soon, and this will take up most of my time then, hence I'd like to get these changes out.

I think @casperklein will share the same stance for this PR, merging should be ok and we can still provide review.

👍🏼

Likewise, review of any follow-up work may take some time, so hopefully it's not too much more to review. I think you mostly wanted to address some rspamd PRs and publish v12.1, and presumably get v13 out before your masters thesis starts?

Just v12.1.0. v13 can take all the time it needs.

If those PRs keep changes isolated to rspamd, it's easier for me to approve with a deferred review (but realistically, right now I may not manage to get around to it). If the review process is adding too much friction, we could consider bypassing review for your PRs, but I think the quality control / feedback of reviews is better than rushing changes.

All the follow-up PRs are just for Rspamd, and they do not touch other parts :) I thought about merging without reviews, but I'm really not sure about such a thing. I think the changes (including this PR) until v12.1.0 are approximately +450/-100 lines. Many additions because I wrote more tests (upcoming PR will just merge the new tests).

I try to make time for changes that are potentially disruptive / complicated so we avoid breaking things. I would love to be more involved in the rspamd feature work, but can't afford more distractions 😅

Rspamd will see just 1 change one could consider breaking (moving a single file location, upcoming PR), but the log shows a warning and all is well. Other than that, it's cleanup, more checks, and improving docs. Few new features are added now (just the rspamd/ directory (this PR) and I have a new DKIM script for Rspamd (upcoming)). All of this is isolated to Rspamd though.

[casperklein]

Hey @georglauterbach, if you are in urge with your thesis, then go merge it 😎

Otherwise I will promise to review at saturday, maybe tomorrow.

It is much easier when the scope is minimized. I know as the contributor it's annoying juggling multiple PRs, but on the reviewer end it's faster when the diff is small and focused.

👍

[georglauterbach]

[...] I will promise to review at saturday, maybe tomorrow.

I guess this is absolutely fine :)

I have a few changes that are basically just adding tests, and I'll not be bothering you with these afterwards :)

[casperklein]

LGTM, just some suggestions/questions:

First I thought, you forgot the dollar sign when passing the variable value to the function:
_env_var_expect_zero_or_one 'ENABLE_RSPAMD'

But then I saw this:

function _env_var_expect_zero_or_one() {
  local ENV_VAR_NAME=${1}
  [[ ${!ENV_VAR_NAME} =~ ^(0|1)$ ]] && return 0

What's your rational for using ${!ENV_VAR_NAME}? Why not just pass the value the the function like we normally do? Seems unnecessary complicated to me 🤷

[georglauterbach]

What's your rational for using ${!ENV_VAR_NAME}? Why not just pass the value the the function like we normally do? Seems unnecessary complicated to me 🤷

Because it allows the log message that comes afterwards: logging the name of the variable that "violates the contract" is something I wanted, so users know exactly which variable is set incorrectly:)

[casperklein]

I see, that makes sense. Are you going to remove the unnecessary exports?

[georglauterbach]

I see, that makes sense. Are you going to remove the unnecessary exports?

Yes, just haven't had the opportunity yet :)

[georglauterbach]

FYI: I enabled auto-merge, so when you approve, this PR will be merged.

[github-actions]

Documentation preview for this PR is ready! 🎉

Built with commit: 628a8fb