Added missing files for deb package #52
Conversation
Declarative sysusers setup in rpm package Minor fixes to rpm and deb service unit files
WalkthroughAdds packaging and automation for certificate renewal: new systemd oneshot service and weekly timer, sysusers config and spec conditionals for RPM builds, a Debian postrm purge step, Makefile RPM artifact copy changes, and .gitignore simplified to generic built-artifact patterns. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Timer as systemd.timer
participant Service as systemd.service (dnstapir-renew)
participant CLI as dnstapir-cli
participant FS as /etc/dnstapir/certs
Note over Timer: Weekly trigger (OnCalendar=Weekly, RandomizedDelaySec)
Timer->>Service: start
Service->>FS: check files (datakey-priv.json, ca.crt, tls.key, tls.crt)
alt files present
Service->>CLI: run `dnstapir-cli renew --generate-ca --generate-tls-client --generate-tls-cert --cert-dir /etc/dnstapir/certs`
CLI-->FS: create/update certs & keys
Service-->>Timer: exit success
else missing files
Service-->>Timer: exit (precondition failed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
deb/DEBIAN/postrm (1)
4-10: Consider handling additional postrm cases for robustness.Debian policy recommends handling additional arguments beyond
removeandpurge:upgrade,failed-upgrade,abort-install,abort-upgrade, anddisappear. While the current implementation won't fail (unhandled cases fall through), explicitly handling or documenting the expected behavior improves maintainability.case "$1" in remove) ;; purge) rm -rf /etc/dnstapir/ ;; + upgrade|failed-upgrade|abort-install|abort-upgrade|disappear) + ;; + *) + echo "postrm called with unknown argument \`$1'" >&2 + exit 1 + ;; esacdeb/usr/lib/systemd/system/dnstapir-renew.timer (1)
8-11: Consider addingPersistent=trueto catch up on missed runs.Without
Persistent=true, if the system is powered off during the scheduled renewal window, the renewal is skipped until the next week. For certificate renewal, addingPersistent=trueensures a missed run triggers immediately after boot.[Timer] OnCalendar=weekly AccuracySec=1h RandomizedDelaySec=100min +Persistent=truedeb/usr/lib/systemd/system/dnstapir-renew.service (1)
3-3: AddWants=network-online.targetto ensure network is ready.
After=network-online.targetonly orders the service after the target but doesn't ensure it's reached. AddingWants=ensures the network is actually online before the renewal attempts to contact the CA.[Unit] Description=DNS TAPIR Edge Certificate Renewal +Wants=network-online.target After=network-online.target
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)Makefile(1 hunks)deb/DEBIAN/postrm(1 hunks)deb/usr/lib/systemd/system/dnstapir-renew.service(1 hunks)deb/usr/lib/systemd/system/dnstapir-renew.timer(1 hunks)rpm/SOURCES/dnstapir-renew.sysusers.conf(1 hunks)rpm/SPECS/dnstapir-cli.spec.in(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:5-6
Timestamp: 2025-08-19T07:59:26.767Z
Learning: The user (zluudg) in dnstapir/cli wants automatic pull-in of tapir-restartpop.service when tapir-renew.service is activated, with failure gating to prevent restarts on failed renewals. This updates the previous preference for manual-only activation.
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/restart-pop.service:11-11
Timestamp: 2025-08-14T11:59:39.396Z
Learning: The restart-pop.service in dnstapir/cli is intended as an optional, manually-activated workaround for restarting tapir-pop.service after certificate renewal. The user (zluudg) prefers to keep tapir-renew.service untouched and is okay with manual activation rather than automatic triggering.
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:0-0
Timestamp: 2025-08-19T08:25:08.604Z
Learning: User zluudg in dnstapir/cli decided to skip failure gating for tapir-restartpop.service, preferring simplicity over complex error handling when automatically triggering service restarts after certificate renewal.
📚 Learning: 2025-08-14T11:59:39.396Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/restart-pop.service:11-11
Timestamp: 2025-08-14T11:59:39.396Z
Learning: The restart-pop.service in dnstapir/cli is intended as an optional, manually-activated workaround for restarting tapir-pop.service after certificate renewal. The user (zluudg) prefers to keep tapir-renew.service untouched and is okay with manual activation rather than automatic triggering.
Applied to files:
deb/usr/lib/systemd/system/dnstapir-renew.timerdeb/usr/lib/systemd/system/dnstapir-renew.servicerpm/SOURCES/dnstapir-renew.sysusers.confrpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-19T08:25:08.604Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:0-0
Timestamp: 2025-08-19T08:25:08.604Z
Learning: User zluudg in dnstapir/cli decided to skip failure gating for tapir-restartpop.service, preferring simplicity over complex error handling when automatically triggering service restarts after certificate renewal.
Applied to files:
deb/usr/lib/systemd/system/dnstapir-renew.timerdeb/usr/lib/systemd/system/dnstapir-renew.servicerpm/SOURCES/dnstapir-renew.sysusers.conf
📚 Learning: 2025-08-19T07:59:26.767Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:5-6
Timestamp: 2025-08-19T07:59:26.767Z
Learning: The user (zluudg) in dnstapir/cli wants automatic pull-in of tapir-restartpop.service when tapir-renew.service is activated, with failure gating to prevent restarts on failed renewals. This updates the previous preference for manual-only activation.
Applied to files:
deb/usr/lib/systemd/system/dnstapir-renew.timerdeb/usr/lib/systemd/system/dnstapir-renew.servicerpm/SOURCES/dnstapir-renew.sysusers.conf
📚 Learning: 2025-08-14T12:03:40.980Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SPECS/tapir-cli.spec:42-42
Timestamp: 2025-08-14T12:03:40.980Z
Learning: User zluudg prefers not to auto-start or auto-enable systemd services in the dnstapir/cli RPM package, preferring manual activation instead.
Applied to files:
deb/usr/lib/systemd/system/dnstapir-renew.servicerpm/SOURCES/dnstapir-renew.sysusers.confrpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-06-09T08:00:25.912Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 33
File: rpm/SPECS/tapir-cli.spec:0-0
Timestamp: 2025-06-09T08:00:25.912Z
Learning: In the dnstapir/cli project, the RPM spec file version field should retain the 'v' prefix (e.g., "Version: v0.3") rather than following the typical RPM convention of omitting it.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
🔇 Additional comments (5)
.gitignore (1)
34-36: LGTM!The simplified artifact patterns appropriately cover build outputs without needing directory-specific rules.
deb/usr/lib/systemd/system/dnstapir-renew.timer (1)
3-6: ConditionPathExists on timer prevents scheduling until files exist.Placing conditions on the timer unit means it won't schedule at all if files are missing during initial activation. This is likely intentional to require initial setup, but be aware the timer needs to be re-enabled after the initial certificate setup.
If you want the timer to always run (letting the service handle missing files gracefully), move these conditions to the service unit instead.
deb/usr/lib/systemd/system/dnstapir-renew.service (1)
5-13: Service configuration looks correct.The oneshot service with dedicated user/group is appropriate for certificate renewal. Based on learnings from PR #41, the RPM version includes automatic pull-in of a restart service after renewal. If the same behavior is desired for Debian, consider adding a similar mechanism.
rpm/SPECS/dnstapir-cli.spec.in (2)
46-48: LGTM!The sysusers.conf installation with correct permissions and destination directory is appropriate.
59-63: Manual fallback correctly adds user to group.Note that the manual
useradd -G dnstapircorrectly adds the user to thednstapirgroup, but as flagged in the sysusers.conf review, the declarative configuration doesn't replicate this membership. Ensure both paths produce the same result.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rpm/SPECS/dnstapir-cli.spec.in (1)
4-9: sysusers_compat condition still uses overly broad version ranges.The
sysusers_compatgating still treats RHEL 8–10, Mageia 8–9, and SUSE <1660 as lacking rpm/sysusers integration. That’s stricter than needed and means you keep falling back to manual user creation on distros that already support declarative sysusers.d. Please update the thresholds as already suggested in the earlier review (e.g. keepfedora < 42, but narrow torhel < 8,mageia < 8, and a tighter/verified SUSE cutoff such as< 1540, or drop the SUSE clause until verified).-%if 0%{?fedora} < 42 || (0%{?rhel} && 0%{?rhel} <= 10) || (0%{?mageia} && 0%{?mageia} < 10) || (0%{?suse_version} && 0%{?suse_version} < 1660) +%if 0%{?fedora} < 42 || (0%{?rhel} && 0%{?rhel} < 8) || (0%{?mageia} && 0%{?mageia} < 8) || (0%{?suse_version} && 0%{?suse_version} < 1540)
🧹 Nitpick comments (2)
rpm/SPECS/dnstapir-cli.spec.in (2)
25-28: SUSE‑only user()/group() Provides — confirm this is intentional.The
Provides: user(dnstapir-renew)/group(dnstapir)are currently emitted only when bothwith sysusers_compatandsuse_versionare set. If you intend these virtual provides to be SUSE‑specific, this is fine. If you’d like them available on other RPM families whensysusers_compatis on, consider dropping the&& 0%{?suse_version}guard.
63-63: Consider making sysusers.d file ownership/permissions explicit.Right now the
%filesentry relies on defaults. For clarity and to avoid surprises across distros, you may want to declare attrs explicitly:-%{_sysusersdir}/dnstapir-renew.conf +%attr(0644,root,root) %{_sysusersdir}/dnstapir-renew.conf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rpm/SPECS/dnstapir-cli.spec.in(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:0-0
Timestamp: 2025-08-19T08:25:08.604Z
Learning: User zluudg in dnstapir/cli decided to skip failure gating for tapir-restartpop.service, preferring simplicity over complex error handling when automatically triggering service restarts after certificate renewal.
📚 Learning: 2025-06-09T08:00:25.912Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 33
File: rpm/SPECS/tapir-cli.spec:0-0
Timestamp: 2025-06-09T08:00:25.912Z
Learning: In the dnstapir/cli project, the RPM spec file version field should retain the 'v' prefix (e.g., "Version: v0.3") rather than following the typical RPM convention of omitting it.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-14T12:03:40.980Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SPECS/tapir-cli.spec:42-42
Timestamp: 2025-08-14T12:03:40.980Z
Learning: User zluudg prefers not to auto-start or auto-enable systemd services in the dnstapir/cli RPM package, preferring manual activation instead.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-19T07:59:26.767Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:5-6
Timestamp: 2025-08-19T07:59:26.767Z
Learning: The user (zluudg) in dnstapir/cli wants automatic pull-in of tapir-restartpop.service when tapir-renew.service is activated, with failure gating to prevent restarts on failed renewals. This updates the previous preference for manual-only activation.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-14T11:59:39.396Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/restart-pop.service:11-11
Timestamp: 2025-08-14T11:59:39.396Z
Learning: The restart-pop.service in dnstapir/cli is intended as an optional, manually-activated workaround for restarting tapir-pop.service after certificate renewal. The user (zluudg) prefers to keep tapir-renew.service untouched and is okay with manual activation rather than automatic triggering.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
🔇 Additional comments (3)
rpm/SPECS/dnstapir-cli.spec.in (3)
21-21: New Source3 entry looks fine; just ensure the file is wired into the build.Declaring
Source3: dnstapir-renew.sysusers.confis consistent with the later install/files usage. Just double‑check that this file is present inrpm/SOURCES(or otherwise available to rpmbuild) so the spec doesn’t fail at source expansion.
52-54: Sysusers config installation path looks correct.Installing
dnstapir-renew.sysusers.confinto%{_sysusersdir}withinstall -m 0644 -Daligns with how sysusers.d snippets are typically packaged; no issues from my side here.
65-69: Gating manual user/group creation behind sysusers_compat looks consistent.Wrapping the existing
%preuser/group creation in%if %{with sysusers_compat}ensures the manualgroupadd/useraddpath is only used on “compat” builds, letting rpm/sysusers handle it elsewhere. This aligns with the intended split between legacy and modern sysusers-capable systems.
zluudg
force-pushed
the
leon/fix-f43-package-fail
branch
from
0871a86 to
59e4cfa
Compare
johanix
left a comment
johanix
left a comment
There was a problem hiding this comment.
As I don't understand any of the RPM magic I cannot say much more than "LGTM" :-)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rpm/SPECS/dnstapir-cli.spec.in(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:5-6
Timestamp: 2025-08-19T07:59:26.767Z
Learning: The user (zluudg) in dnstapir/cli wants automatic pull-in of tapir-restartpop.service when tapir-renew.service is activated, with failure gating to prevent restarts on failed renewals. This updates the previous preference for manual-only activation.
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:0-0
Timestamp: 2025-08-19T08:25:08.604Z
Learning: User zluudg in dnstapir/cli decided to skip failure gating for tapir-restartpop.service, preferring simplicity over complex error handling when automatically triggering service restarts after certificate renewal.
Learnt from: zluudg
Repo: dnstapir/cli PR: 52
File: rpm/SPECS/dnstapir-cli.spec.in:4-9
Timestamp: 2025-12-03T13:05:12.423Z
Learning: In the dnstapir/cli RPM spec file, the sysusers_compat conditional version thresholds (Fedora < 42, RHEL <= 10, Mageia < 10, SUSE < 1660) are based on RPM v4.20+ native sysusers.d support, not just systemd-sysusers availability. These thresholds follow the authoritative reference from Fedora's mock.spec package.
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SPECS/tapir-cli.spec:42-42
Timestamp: 2025-08-14T12:03:40.980Z
Learning: User zluudg prefers not to auto-start or auto-enable systemd services in the dnstapir/cli RPM package, preferring manual activation instead.
📚 Learning: 2025-12-03T13:05:12.423Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 52
File: rpm/SPECS/dnstapir-cli.spec.in:4-9
Timestamp: 2025-12-03T13:05:12.423Z
Learning: In the dnstapir/cli RPM spec file, the sysusers_compat conditional version thresholds (Fedora < 42, RHEL <= 10, Mageia < 10, SUSE < 1660) are based on RPM v4.20+ native sysusers.d support, not just systemd-sysusers availability. These thresholds follow the authoritative reference from Fedora's mock.spec package.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-06-09T08:00:25.912Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 33
File: rpm/SPECS/tapir-cli.spec:0-0
Timestamp: 2025-06-09T08:00:25.912Z
Learning: In the dnstapir/cli project, the RPM spec file version field should retain the 'v' prefix (e.g., "Version: v0.3") rather than following the typical RPM convention of omitting it.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-14T12:03:40.980Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SPECS/tapir-cli.spec:42-42
Timestamp: 2025-08-14T12:03:40.980Z
Learning: User zluudg prefers not to auto-start or auto-enable systemd services in the dnstapir/cli RPM package, preferring manual activation instead.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-19T07:59:26.767Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:5-6
Timestamp: 2025-08-19T07:59:26.767Z
Learning: The user (zluudg) in dnstapir/cli wants automatic pull-in of tapir-restartpop.service when tapir-renew.service is activated, with failure gating to prevent restarts on failed renewals. This updates the previous preference for manual-only activation.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-19T08:25:08.604Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/tapir-renew.service:0-0
Timestamp: 2025-08-19T08:25:08.604Z
Learning: User zluudg in dnstapir/cli decided to skip failure gating for tapir-restartpop.service, preferring simplicity over complex error handling when automatically triggering service restarts after certificate renewal.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
📚 Learning: 2025-08-14T11:59:39.396Z
Learnt from: zluudg
Repo: dnstapir/cli PR: 41
File: rpm/SOURCES/restart-pop.service:11-11
Timestamp: 2025-08-14T11:59:39.396Z
Learning: The restart-pop.service in dnstapir/cli is intended as an optional, manually-activated workaround for restarting tapir-pop.service after certificate renewal. The user (zluudg) prefers to keep tapir-renew.service untouched and is okay with manual activation rather than automatic triggering.
Applied to files:
rpm/SPECS/dnstapir-cli.spec.in
🔇 Additional comments (6)
rpm/SPECS/dnstapir-cli.spec.in (6)
21-21: LGTM!The addition of the sysusers configuration source file is correct and necessary for declarative user/group management on distributions with RPM v4.20+ native sysusers support.
34-35: LGTM!The path definitions are correct. Removing the trailing slash from
_unitdirnormalizes the path, and adding_sysusersdiris necessary for installing the sysusers configuration file.
52-53: LGTM!The sysusers configuration file installation is correct. The file is always installed (as it should be), and RPM v4.20+ will process it automatically on supported distributions, while older distributions will fall back to the conditional
%prescript.
63-63: LGTM!The file list entry is correct with appropriate permissions (0644) and ownership (root:root) for a sysusers.d configuration file.
65-69: LGTM!The conditional wrapping of the
%prescript is correct. On distributions with RPM v4.20+ native sysusers support, the declarative sysusers.d file will handle user/group creation. On older distributions, the traditional%prescriptlet will execute as a fallback.
25-28: Confirm whether Provides tags should apply to all sysusers_compat distributions, not just SUSE.RPM (since v4.19+) automatically generates virtual
Provides: user(<name>)andgroup(<name>)for users and groups declared in sysusers.d files across all distributions (Fedora, RHEL, Mageia, SUSE). This is not SUSE-specific behavior. Since thesysusers_compatconditional targets distributions at versions where RPM v4.20+ native sysusers.d support may be unavailable, explicitly declaring these Provides ensures compatibility on those older systems. The current code restricts these tags to SUSE only—clarify whether Fedora, RHEL, and Mageia at their respective sysusers_compat thresholds should also have explicit Provides declarations for the same compatibility reason.
Declarative sysusers setup in rpm package
Minor fixes to rpm and deb service unit files
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.