[__opendkim*] add debian support #32

Open
fnux wants to merge 2 commits from opendkim-debian into master
Collaborator

See also: #28

See also: #28
fnux added 2 commits 2024-05-24 07:37:33 +00:00
evilham requested changes 2024-05-24 12:38:25 +00:00
evilham left a comment
Collaborator

Hey, the general gist of this review is:

  • maybe instead of adding a parameter that we may or may not want (--pidfile), we should consider re-using --custom-config and customising that when adding the defaults for Debian
  • there seems to be a reliance on --userid, which is marked as deprecated in the type, when using Debian
Hey, the general gist of this review is: - maybe instead of adding a parameter that we may or may not want (`--pidfile`), we should consider re-using `--custom-config` and customising that when adding the defaults for Debian - there seems to be a reliance on `--userid`, which is marked as deprecated in the type, when using Debian
@ -4,2 +4,4 @@
echo "# Managed remotely, manual changes will be lost."
# Used for OS-specific configuration.
os=$(cat "${__global:?}/explorer/os")
Collaborator

It seems like we are not using this really, can we remove it?

It seems like we are not using this really, can we remove it?
@ -16,3 +16,2 @@
Note that this type is currently only implemented for Alpine Linux and FreeBSD.
Please contribute an implementation if you can.
Note that this type is currently only implemented for Debian, Alpine Linux and
Collaborator

Nit-pick: could we write things alphabetically here? :-D

Nit-pick: could we write things alphabetically here? :-D
@ -29,2 +33,4 @@
;;
'freebsd')
CFG_DIR="/usr/local/etc/mail"
CFG_FILE="$CFG_DIR/opendkim.conf"
Collaborator

This seems redundant as it is the default.

This seems redundant as it is the default.
@ -73,0 +91,4 @@
# "opendkim" in that case.
if [ -z "$USERID" ]; then
export USERID="opendkim"
fi
Collaborator

Given that USERID exists for the --userid parameter, which was deprecated, I think this should instead (or additionally) check for the UserID being defined in --extra-config, and if not, appending the corresponding line; similar to --processid.

At least in theory, removing support for --userid should keep things working.

As a general rule for all these special cases (--umask and --userid, for --pidfile see comment below): they should be mentioned in man.rst as special default values.

Given that `USERID` exists for the `--userid` parameter, which was deprecated, I think this should instead (or additionally) check for the `UserID` being defined in `--extra-config`, and if not, appending the corresponding line; similar to `--processid`. At least in theory, removing support for `--userid` should keep things working. As a general rule for all these special cases (`--umask` and `--userid`, for `--pidfile` see comment below): they should be mentioned in `man.rst` as special default values.
@ -4,3 +4,4 @@ subdomains
umask
userid
custom-config
pidfile
Collaborator

Rather than add more parameters to the type (we don't want to support 100% of the .conf), I'd propose we rely on --custom-config instead.

We can check the contents of --custom-config and adapt that as necessary for e.g. Debian.

This keeps the type smaller and still just as flexible and out-of-the-box.

Rather than add more parameters to the type (we don't want to support 100% of the `.conf`), I'd propose we rely on `--custom-config` instead. We can check the contents of `--custom-config` and adapt that as necessary for e.g. Debian. This keeps the type smaller and still just as flexible and out-of-the-box.
@ -3,0 +9,4 @@
DIRECTORY="/var/db/dkim/"
;;
*)
DIRECTORY="/var/db/dkim/"
Collaborator

I'd probably propose we keep only the *) case, or we leave DIRECTORY="/var/db/dkim/" outside of the case block, and only keep the 'debian') case.

I'd probably propose we keep only the `*)` case, or we leave `DIRECTORY="/var/db/dkim/"` outside of the `case` block, and only keep the `'debian')` case.
@ -23,3 +23,3 @@
`--selector` parameters, the `$__object_id` must be in form `$domain/$selector`.
Currently, this type is only implemented for Alpine Linux and FreeBSD.
Currently, this type is only implemented for Debian, Alpine Linux and FreeBSD.
Collaborator

Same nit-pick as for __opendkim, I'd suggest using alphabetical sorting.

Same nit-pick as for `__opendkim`, I'd suggest using alphabetical sorting.
This pull request is broken due to missing fork information.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin opendkim-debian:opendkim-debian
git checkout opendkim-debian

Merge

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff opendkim-debian
git checkout master
git merge --ff-only opendkim-debian
git checkout opendkim-debian
git rebase master
git checkout master
git merge --no-ff opendkim-debian
git checkout master
git merge --squash opendkim-debian
git checkout master
git merge --ff-only opendkim-debian
git checkout master
git merge opendkim-debian
git push origin master
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: ungleich-public/cdist-contrib#32
No description provided.