[__opendkim*] add debian support #32

fnux · 2024-05-24T07:37:33Z

fnux commented

2024-05-24 07:37:33 +00:00

See also: #28

fnux added 2 commits

2024-05-24 07:37:33 +00:00

[__opendkim] add debian support d97fb9a434

[__opendkim_genkey] add debian support 40d7b4354e

fnux referenced this pull request

2024-05-24 07:37:50 +00:00

[__opendkim*] add debian support #28

evilham requested changes

2024-05-24 12:38:25 +00:00

evilham left a comment

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

type/__opendkim/files/opendkim.conf.sh

					
				@ -4,2 +4,4 @@

				echo "# Managed remotely, manual changes will be lost."

				# Used for OS-specific configuration.

				os=$(cat "${__global:?}/explorer/os")

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

type/__opendkim/man.rst

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

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

type/__opendkim/manifest

					
				@ -29,2 +33,4 @@

					;;

				'freebsd')

					CFG_DIR="/usr/local/etc/mail"

					CFG_FILE="$CFG_DIR/opendkim.conf"

This seems redundant as it is the default.

type/__opendkim/manifest

					
				@ -73,0 +91,4 @@

					# "opendkim" in that case.

					if [ -z "$USERID" ]; then

						export USERID="opendkim"

					fi

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.

type/__opendkim/parameter/optional

					
				@ -4,3 +4,4 @@ subdomains

				umask

				userid

				custom-config

				pidfile

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.

type/__opendkim_genkey/explorer/key-state

					
				@ -3,0 +9,4 @@

					DIRECTORY="/var/db/dkim/"

				;;

				*)

					DIRECTORY="/var/db/dkim/"

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.

type/__opendkim_genkey/man.rst

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

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.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin opendkim-debian:opendkim-debian

git switch opendkim-debian

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master

git merge --no-ff opendkim-debian

git switch opendkim-debian

git rebase master

git switch master

git merge --ff-only opendkim-debian

git switch opendkim-debian

git rebase master

git switch master

git merge --no-ff opendkim-debian

git switch master

git merge --squash opendkim-debian