Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm Description: A pure Unix shell script implementing ACME client protocol. - An ACME protocol client written purely in Shell (Unix shell) language. - Full ACME protocol implementation. - Support ECDSA certs - Support SAN and wildcard certs - Simple, powerful and very easy to use. You only need 3 minutes to learn it. - Bash, dash and sh compatible. - Purely written in Shell with no dependencies on python. - Just one script to issue, renew and install your certificates automatically. - DOES NOT require root/sudoer access. - Docker ready - IPv6 ready - Cron job notifications for renewal or error etc. Fedora Account System Username: mikelo2
Hi! I'm new to Fedora Packaging and reviewing some packages as part of working towards sponsorship into the packager group. I use `acme.sh` quite a bit myself, so it seemed fitting to review this one. I can't perform binding reviews yet, but hopefully this helps. I'll post some things I spotted this comment and then I can post the full fedora-review checklist in the future. 1. The License field isn't a valid SPDX expression because of the use of `GPL-3.0` based on https://spdx.org/licenses/ this needs to be either `GPL-3.0-only` or `GPL-3.0-or-later`. 2. From what I understand, the systemd_post scriptlet needs activating in the %post section too, even if the timer isn't being started by default. Other than that, this is looking pretty good to me so far.
Thanks Alex for your review! Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm - Changed license to GPL-3.0-or-later - Added systemd_post scriplet
Copr build: https://copr.fedorainfracloud.org/coprs/build/9150225 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2370890-acme.sh/fedora-rawhide-x86_64/09150225-acme.sh/fedora-review/review.txt Found issues: - Systemd service file(s) in acme.sh Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm Another try to check if the systemd issue is gone
Created attachment 2093565 [details] The .spec file difference from Copr build 9150225 to 9150712
Copr build: https://copr.fedorainfracloud.org/coprs/build/9150712 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2370890-acme.sh/fedora-rawhide-x86_64/09150712-acme.sh/fedora-review/review.txt Found issues: - Systemd service file(s) in acme.sh Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Just a few comments: ``` %post %systemd_post acmesh-renew.timer # Remind users to start acmesh-renew.timer if they need acme.sh to automatically renew certs if [ "$1" -eq 1 ] ; then echo "" echo "acme.sh auto renewal timer is not started by default." echo "Run 'systemctl start acmesh-renew.timer' to enable automatic renewals." fi %preun %systemd_preun acmesh-renew.timer %postun %systemd_postun_with_restart acmesh-renew.timer ``` I'm pretty sure this not what you want here. I don't think restarting a timer doesn't do anything ... and printing output in RPM scriptlets is very frowned upon. I would just do what's documented in the Packaging Guidelines for systemd services: ``` %post %systemd_post acmesh-renew.service %preun %systemd_preun acmesh-renew.service %postun %systemd_postun_with_restart acmesh-renew.service ```
Spec URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh.spec SRPM URL: https://mikel.olasagasti.info/tmp/fedora/acme.sh-3.1.1-1.fc42.src.rpm I copied the way certbot package is doing: https://src.fedoraproject.org/rpms/certbot/blob/rawhide/f/certbot.spec#_273-293 But agree that it might not be the best option, so new spec changes that.