dist: add missing timer and service units to snapm.spec#445
Conversation
WalkthroughThe spec file now installs four systemd unit files (two services and two timers) into Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
snapm.spec (1)
95-100: Units correctly installed into %{_unitdir}; minor packaging nits
- Good: copies all four units into the correct dir with 0644 perms.
- Optional: use install -D to drop the separate mkdir and make destinations explicit.
Apply if you want the tighter style:
-mkdir -p ${RPM_BUILD_ROOT}/%{_unitdir} -%{__install} -p -m 644 systemd/snapm-create@.service ${RPM_BUILD_ROOT}/%{_unitdir} -%{__install} -p -m 644 systemd/snapm-create@.timer ${RPM_BUILD_ROOT}/%{_unitdir} -%{__install} -p -m 644 systemd/snapm-gc@.service ${RPM_BUILD_ROOT}/%{_unitdir} -%{__install} -p -m 644 systemd/snapm-gc@.timer ${RPM_BUILD_ROOT}/%{_unitdir} +%{__install} -D -m 0644 systemd/snapm-create@.service ${RPM_BUILD_ROOT}/%{_unitdir}/snapm-create@.service +%{__install} -D -m 0644 systemd/snapm-create@.timer ${RPM_BUILD_ROOT}/%{_unitdir}/snapm-create@.timer +%{__install} -D -m 0644 systemd/snapm-gc@.service ${RPM_BUILD_ROOT}/%{_unitdir}/snapm-gc@.service +%{__install} -D -m 0644 systemd/snapm-gc@.timer ${RPM_BUILD_ROOT}/%{_unitdir}/snapm-gc@.timerAdditionally, if targeting older distros without systemd file triggers, consider adding daemon-reload scriptlets (omit restart for templated units):
+%{?systemd_requires} + +%post +%systemd_post snapm-create@.timer snapm-gc@.timer snapm-create@.service snapm-gc@.service +%preun +%systemd_preun snapm-create@.timer snapm-gc@.timer snapm-create@.service snapm-gc@.service +%postun +%systemd_postun snapm-create@.timer snapm-gc@.timer snapm-create@.service snapm-gc@.servicePlease confirm your target build roots (e.g., Fedora/EPEL versions) to decide whether these scriptlets are necessary given systemd file triggers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
snapm.spec(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: python-basic-tests
|
@coderabbitai two things regarding nitpick 1:
|
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru Thank you for the clarification and technical context! I appreciate you explaining the rationale behind the packaging approach. You're absolutely right about systemd file triggers being well-established, and I understand now that the upstream I'll keep these packaging preferences and scope considerations in mind for future reviews of this codebase. ✏️ Learnings added
|
bmr-cymru
force-pushed
the
bmr-fix-systemd-unit-install
branch
from
56b2cbb to
92428f6
Compare
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru Thank you for the detailed clarification and technical context! I appreciate you taking the time to explain the rationale behind the packaging approach. You're absolutely right about systemd file triggers being well-established - that Fedora documentation confirms they've been around for quite some time and are a reliable mechanism. I also understand now that the upstream The preference for the explicit I have this context captured in my learnings and will keep these packaging preferences and scope considerations in mind for future reviews of this codebase. Thanks for the educational insight! 🧠 Learnings used |
1 participant
Resolves: #443
Summary by CodeRabbit
New Features
Chores