ungleich-public/cdist-contrib
ungleich-public/cdist-contrib
8
2
Fork 3
Code Issues 2 Pull requests 3 Projects Releases Wiki Activity

[__opendkim*] add debian support #32

Open
fnux wants to merge 2 commits from opendkim-debian into master
pull from: opendkim-debian
merge into: ungleich-public:master
Conversation 0 Commits 2 Files changed 7 +84 -33
fnux commented 2024-05-24 07:37:33 +00:00
Collaborator
Copy link

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
Copy link

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")
evilham commented 2024-05-24 11:48:13 +00:00
Collaborator
Copy link

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?
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
evilham commented 2024-05-24 11:46:48 +00:00
Collaborator
Copy link

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

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"
evilham commented 2024-05-24 12:22:57 +00:00
Collaborator
Copy link

This seems redundant as it is the default.

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
evilham commented 2024-05-24 12:15:20 +00:00
Collaborator
Copy link

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
evilham commented 2024-05-24 12:35:14 +00:00
Collaborator
Copy link

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/"
evilham commented 2024-05-24 12:24:55 +00:00
Collaborator
Copy link

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.
evilham commented 2024-05-24 11:58:55 +00:00
Collaborator
Copy link

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 checkout 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 checkout master
git merge --no-ff opendkim-debian
git checkout opendkim-debian
git rebase master
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.
Reviewers
No reviewers
evilham
Labels
Clear labels   
bug

   
confirmed

   
critical

   
discussion

   
documentation

   
enhancement

   
feedback-required

   
good-first-issue

   
ready

   
reviewed

   
suggestion

   
support
No labels
bug
confirmed
critical
discussion
documentation
enhancement
feedback-required
good-first-issue
ready
reviewed
suggestion
support
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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.
Delete branch "opendkim-debian"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?