search

lelutin / puppet-spamassassin

Install and configure spamassassin either as a service (through spamd) or for use with applications like amavisd. This is a fork of https://bitbucket.org/wyrie/puppet-spamassassin
Other
2 stars 9 forks source link

Add support for a custom service name + Debian 12 support #36

Closed Arakmar closed 4 months ago

Arakmar commented 1 year ago

Since Debian 12, the service name changed from spamassassin to spamd

lelutin commented 1 year ago

hi @Arakmar

thanks for this pull request and helping me to keep support up to date in debian, I really appreciate it :)

CI has noted something that's missing in your PR: could you add to the comments at the top of manifests/init.pp a section that documents the new option? (just copy the style of comment from the other options).

p.s.: don't pay any mind to the option notify_service_name while documenting, it looks very similar to what you've added, but I just took a look and it's something that was rendered obsolete/useless when I changed to using contain and ordering in the main class but I forgot to remove the parameter -- once your PR is merged I'll take care of removing that bogus parameter.

I've run other checks that CI will be running and so far I don't see other issues than the missing param documentaton.

I've noticed something that I think we should add to your work: now that you've mentioned the service name change, I've taken a look at the packages in debian and it seems as though the spamd daemon comes from the new spamd package (which in turn pulls in the spamassassin package). I think we also want to parametrize the package name so that we can use spamd in debian 12 instead of spamassassin. Would you mind adding that in? if it's too bothersome I can manage this part later.

Last but not least, there another change that's missing, but it's a bit more like a "reward": we should tag debian 12 as supported by this module. I suggest that we add version 12 to the operating_support option inside metadata.json. That'll broadcast to all on forge that the module does support debian 12 :)

Cheers!

Arakmar commented 1 year ago

Hi, thanks for the review :) I totally forgot about the documentation, shame on me ! I also added the package parameter (with the documentation) and updated the Debian version in the metadata file.

Arakmar commented 1 year ago

The CI should be green now, missing trailing comma ;)

lelutin commented 1 year ago

@Arakmar oh haha didn't think you'd fix the CI issue so fast :) I had changed it up already to avoid a round trip since it was just a missing comma :)

thanks a bunch for your work! I've merged your work on master already (from the commit before the last force-push on the branch). I'll try and find some time soon to create a new forge release with this.