carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

updating rpm builds #2251

Merged
mfreeman451 merged 1 commit from refs/pull/2251/head into main 2025-09-29 15:45:29 +00:00
Conversation 2 Commits 1 Files changed 22 +110
mfreeman451 commented 2025-09-29 15:43:13 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #1671
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1671
Original created: 2025-09-29T15:43:13Z
Original updated: 2025-09-29T15:45:33Z
Original head: carverauto/serviceradar:packaging/proton_rpm_broken
Original base: main
Original merged: 2025-09-29T15:45:29Z by @mfreeman451

PR Type

Enhancement

Description

  • Add automatic service management to RPM packages

  • Enable services on fresh install, restart on upgrade

  • Improve systemd integration across all serviceradar components

Diagram Walkthrough

flowchart LR
  A["RPM Install"] --> B["Check Install Type"]
  B --> C["Fresh Install ($1 = 1)"]
  B --> D["Upgrade ($1 > 1)"]
  C --> E["Enable & Start Service"]
  D --> F["Try Restart Service"]
  E --> G["Service Running"]
  F --> G["Service Running"]

File Walkthrough
Relevant files
Enhancement
22 files
serviceradar-agent.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-core.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-event-writer.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-flowgger.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-goflow2.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-kong.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-kv.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-mapper.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-nats.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-otel.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-poller.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-profiler.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-proton.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-rperf-checker.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-rperf.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-snmp-checker.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-srql.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-sync.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-sysmon.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-trapd.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-web.spec
Add service auto-start logic                                                         
+5/-0     
serviceradar-zen.spec
Add service auto-start logic                                                         
+5/-0     
Imported from GitHub pull request. Original GitHub pull request: #1671 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1671 Original created: 2025-09-29T15:43:13Z Original updated: 2025-09-29T15:45:33Z Original head: carverauto/serviceradar:packaging/proton_rpm_broken Original base: main Original merged: 2025-09-29T15:45:29Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Add automatic service management to RPM packages - Enable services on fresh install, restart on upgrade - Improve systemd integration across all serviceradar components ___ ### Diagram Walkthrough ```mermaid flowchart LR A["RPM Install"] --> B["Check Install Type"] B --> C["Fresh Install ($1 = 1)"] B --> D["Upgrade ($1 > 1)"] C --> E["Enable & Start Service"] D --> F["Try Restart Service"] E --> G["Service Running"] F --> G["Service Running"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>22 files</summary><table> <tr> <td><strong>serviceradar-agent.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-9fd8b118e9eb1b76ed4cc7664b788115634d3f1978bcc4fee0ea5873de1f6b90">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-core.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-b03570dbdd332568f77902ae5a6dc38255e2639e1fa4bb921c06c1557dbf35e0">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-event-writer.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-2a02c3cc8b4a3ab0ba76ce10f8c2e9168179e0f9f3eaa19807bb77d9a79b30e9">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-flowgger.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-894a88a48d87c8564c8958dff8a5d186cff83a1b48163182436f7f324f765e8c">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-goflow2.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-bab457844021064fedca41f57b7eca08a506f4985a9364da6e1691b7167aebd9">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-kong.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-e1be0e25d1d167a40c46c2b32773948a0e0df715f27f9cf4c011fec892b27583">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-kv.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-413b49672fc2bf028e0db1cf08f88fdc57608a82e4dfe3aed2cec699d3237d75">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-mapper.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-169bda1e25390e292d863fec1a7e455de7800143e08312ff3f67349b5edd0270">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-nats.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-3341a04774c8bd3113142b3abaab5c5f4db8e12cc6a6a789b30555f602444172">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-otel.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-593ca55958a2e0326e3eadce02fdd31e240e68bdf31fda39b9607d0c23cdf6c4">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-poller.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-afe5af03df8123f1126cf5b790cca19c707968dae11d300dd75a52097323857f">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-profiler.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-268b89d5448fb107cdfe16cc0d9964781c02dc39dc828725315f482fd48e16b6">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-proton.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-6aa5dc112bfc6b27d076f2a6a5f87cf7a285ec70dbf032f720b9b4c9dc646eab">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-rperf-checker.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-fd3217cc52aeca1a10d4826f4b78fd2319a39a11039d798840baab1d0d174875">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-rperf.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-e29a7a6d3f0619a2460280c62bbd7bb8f608fde56c8078041c42cbe41e9ad95e">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-snmp-checker.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-b29e95ee68a3151965ee19d69ed21f519ddf530dde9dfd1ad8d90e88b48717c7">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-srql.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-0f0d2551b721585ca16456a6217f4633c70f56c4a667e721d6783744ca3e5ff8">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-sync.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-ad03df822d4f446590c2e7f35f39921575ea7f7600209eabd02ea4888fd3f8d9">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-sysmon.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-1140c1c7039cda75a719ccf637cec7bb67e49313ebcbe632f34381901c5c5005">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-trapd.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-230b82e75a46737b86cede13d2b7c9a8b63e11f5ecffd8593d37ca8d637ff65e">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-web.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-c84b985ecad45b6ee62657446e763878a6dd4e9a01e5903053cc923d88c34edb">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-zen.spec</strong><dd><code>Add service auto-start logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-b09626493d21438651625c21e3a01752c29aff928093ae915b0f738ae1e4404c">+5/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-09-29 15:43:55 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1671#issuecomment-3347734848
Original created: 2025-09-29T15:43:55Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Macro Duplication
The manual systemctl calls duplicate %systemd_post behavior and may conflict with distro policy; consider using the RPM macros (%systemd_post with Wants=, WantedBy=) or %systemd_post/%systemd_user_post patterns consistently to avoid double reloads or unexpected state.

Conditional Check
The test 'if [ $1 -eq 1 ]' is unquoted; if $1 is empty or unset it can cause a shell error. Use 'if [ "${1:-0}" -eq 1 ]' or case "$1" in 1) ... esac for robustness across script invocations.

Service Enablement
Enabling services on install may surprise users in managed environments; consider gating with a macro/boolean (e.g., %_enable_services) or honoring presets via 'systemctl preset --global' instead of unconditional enable.

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1671#issuecomment-3347734848 Original created: 2025-09-29T15:43:55Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️&nbsp;<strong>Estimated effort to review</strong>: 2 🔵🔵⚪⚪⚪</td></tr> <tr><td>🧪&nbsp;<strong>No relevant tests</strong></td></tr> <tr><td>🔒&nbsp;<strong>No security concerns identified</strong></td></tr> <tr><td>⚡&nbsp;<strong>Recommended focus areas for review</strong><br><br> <a href='https://github.com/carverauto/serviceradar/pull/1671/files#diff-e5e0ccca22046605e83fd1bc4ac253d1e5f24d4c77ea3a83dbf7203f63e9b259R46-R50'><strong>Macro Duplication</strong></a><br>The manual systemctl calls duplicate %systemd_post behavior and may conflict with distro policy; consider using the RPM macros (%systemd_post with Wants=, WantedBy=) or %systemd_post/%systemd_user_post patterns consistently to avoid double reloads or unexpected state. <a href='https://github.com/carverauto/serviceradar/pull/1671/files#diff-e5e0ccca22046605e83fd1bc4ac253d1e5f24d4c77ea3a83dbf7203f63e9b259R46-R50'><strong>Conditional Check</strong></a><br>The test 'if [ $1 -eq 1 ]' is unquoted; if $1 is empty or unset it can cause a shell error. Use 'if [ "${1:-0}" -eq 1 ]' or case "$1" in 1) ... esac for robustness across script invocations. <a href='https://github.com/carverauto/serviceradar/pull/1671/files#diff-e5e0ccca22046605e83fd1bc4ac253d1e5f24d4c77ea3a83dbf7203f63e9b259R46-R50'><strong>Service Enablement</strong></a><br>Enabling services on install may surprise users in managed environments; consider gating with a macro/boolean (e.g., %_enable_services) or honoring presets via 'systemctl preset --global' instead of unconditional enable. </td></tr> </table>
qodo-code-review[bot] commented 2025-09-29 15:45:03 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1671#issuecomment-3347744172
Original created: 2025-09-29T15:45:03Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Use RPM macros to avoid duplication

To avoid code duplication across 22 RPM spec files, define a custom RPM macro
for the repeated service management shell script. This will centralize the
logic, improving maintainability and consistency.

Examples:

packaging/specs/serviceradar-agent.spec [46-50] 
if [ $1 -eq 1 ]; then
    systemctl enable --now serviceradar-agent.service >/dev/null 2>&1 || :
else
    systemctl try-restart serviceradar-agent.service >/dev/null 2>&1 || :
fi
packaging/specs/serviceradar-core.spec [58-62] 
if [ $1 -eq 1 ]; then
    systemctl enable --now serviceradar-core.service >/dev/null 2>&1 || :
else
    systemctl try-restart serviceradar-core.service >/dev/null 2>&1 || :
fi

Solution Walkthrough:

Before:

# In each of the 22 spec files:
%post
%systemd_post my-service.service
if [ $1 -eq 1 ]; then
    systemctl enable --now my-service.service >/dev/null 2>&1 || :
else
    systemctl try-restart my-service.service >/dev/null 2>&1 || :
fi
# ... other post-install commands

After:

# In a central macro definition file:
%serviceradar_handle_service_on_install(s) \
if [ $1 -eq 1 ]; then \
    systemctl enable --now %{s} >/dev/null 2>&1 || :; \
else \
    systemctl try-restart %{s} >/dev/null 2>&1 || :; \
fi

# In each of the 22 spec files:
%post
%systemd_post my-service.service
%serviceradar_handle_service_on_install my-service.service
# ... other post-install commands
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies significant code duplication across 22 files and proposes using a custom RPM macro, which is a best practice for improving maintainability and consistency.

High
Possible issue
Remove redundant service management commands

Remove the redundant systemctl daemon-reload, systemctl enable, and systemctl
start commands from the %post scriptlet. The newly added if block already
handles enabling and starting the service on initial installation.

packaging/specs/serviceradar-poller.spec [39-51]

 %systemd_post serviceradar-poller.service
 if [ $1 -eq 1 ]; then
+    # On initial install, enable and start the service
     systemctl enable --now serviceradar-poller.service >/dev/null 2>&1 || :
-else
-    systemctl try-restart serviceradar-poller.service >/dev/null 2>&1 || :
 fi
 chown -R serviceradar:serviceradar /etc/serviceradar
 chmod 755 /usr/local/bin/serviceradar-poller
 
-# Enable and start service
-systemctl daemon-reload
-systemctl enable serviceradar-poller.service
-systemctl start serviceradar-poller.service
-
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that the PR introduces code that makes existing service management commands (daemon-reload, enable, start) redundant. Removing these lines is a critical cleanup to prevent duplicate operations and potential race conditions during package installation.

Medium
General
Simplify service handling in post-install

In the %post scriptlet, remove the else block containing systemctl try-restart.
This avoids redundant service restarts on upgrade, which should be handled by
the %systemd_postun_with_restart macro.

packaging/specs/serviceradar-agent.spec [46-50]

 if [ $1 -eq 1 ]; then
+    # On initial install, enable and start the service
     systemctl enable --now serviceradar-agent.service >/dev/null 2>&1 || :
-else
-    systemctl try-restart serviceradar-agent.service >/dev/null 2>&1 || :
 fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that using try-restart in the %post scriptlet is redundant if %systemd_postun_with_restart is used for upgrades, which is a standard packaging practice. Removing the else block simplifies the script and avoids a potential double-restart on upgrades.

Low
  • More
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1671#issuecomment-3347744172 Original created: 2025-09-29T15:45:03Z --- ## PR Code Suggestions ✨ <!-- 55fd86e --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Use RPM macros to avoid duplication</summary> ___ **To avoid code duplication across 22 RPM spec files, define a custom RPM macro <br>for the repeated service management shell script. This will centralize the <br>logic, improving maintainability and consistency.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-9fd8b118e9eb1b76ed4cc7664b788115634d3f1978bcc4fee0ea5873de1f6b90R46-R50">packaging/specs/serviceradar-agent.spec [46-50]</a> </summary> ```spec if [ $1 -eq 1 ]; then systemctl enable --now serviceradar-agent.service >/dev/null 2>&1 || : else systemctl try-restart serviceradar-agent.service >/dev/null 2>&1 || : fi ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1671/files#diff-b03570dbdd332568f77902ae5a6dc38255e2639e1fa4bb921c06c1557dbf35e0R58-R62">packaging/specs/serviceradar-core.spec [58-62]</a> </summary> ```spec if [ $1 -eq 1 ]; then systemctl enable --now serviceradar-core.service >/dev/null 2>&1 || : else systemctl try-restart serviceradar-core.service >/dev/null 2>&1 || : fi ``` </details> ### Solution Walkthrough: #### Before: ```spec # In each of the 22 spec files: %post %systemd_post my-service.service if [ $1 -eq 1 ]; then systemctl enable --now my-service.service >/dev/null 2>&1 || : else systemctl try-restart my-service.service >/dev/null 2>&1 || : fi # ... other post-install commands ``` #### After: ```spec # In a central macro definition file: %serviceradar_handle_service_on_install(s) \ if [ $1 -eq 1 ]; then \ systemctl enable --now %{s} >/dev/null 2>&1 || :; \ else \ systemctl try-restart %{s} >/dev/null 2>&1 || :; \ fi # In each of the 22 spec files: %post %systemd_post my-service.service %serviceradar_handle_service_on_install my-service.service # ... other post-install commands ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies significant code duplication across 22 files and proposes using a custom RPM macro, which is a best practice for improving maintainability and consistency. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Remove redundant service management commands</summary> ___ **Remove the redundant <code>systemctl daemon-reload</code>, <code>systemctl enable</code>, and <code>systemctl </code><br><code>start</code> commands from the <code>%post</code> scriptlet. The newly added <code>if</code> block already <br>handles enabling and starting the service on initial installation.** [packaging/specs/serviceradar-poller.spec [39-51]](https://github.com/carverauto/serviceradar/pull/1671/files#diff-afe5af03df8123f1126cf5b790cca19c707968dae11d300dd75a52097323857fR39-R51) ```diff %systemd_post serviceradar-poller.service if [ $1 -eq 1 ]; then + # On initial install, enable and start the service systemctl enable --now serviceradar-poller.service >/dev/null 2>&1 || : -else - systemctl try-restart serviceradar-poller.service >/dev/null 2>&1 || : fi chown -R serviceradar:serviceradar /etc/serviceradar chmod 755 /usr/local/bin/serviceradar-poller -# Enable and start service -systemctl daemon-reload -systemctl enable serviceradar-poller.service -systemctl start serviceradar-poller.service - ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly points out that the PR introduces code that makes existing service management commands (`daemon-reload`, `enable`, `start`) redundant. Removing these lines is a critical cleanup to prevent duplicate operations and potential race conditions during package installation. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Simplify service handling in post-install</summary> ___ **In the <code>%post</code> scriptlet, remove the <code>else</code> block containing <code>systemctl try-restart</code>. <br>This avoids redundant service restarts on upgrade, which should be handled by <br>the <code>%systemd_postun_with_restart</code> macro.** [packaging/specs/serviceradar-agent.spec [46-50]](https://github.com/carverauto/serviceradar/pull/1671/files#diff-9fd8b118e9eb1b76ed4cc7664b788115634d3f1978bcc4fee0ea5873de1f6b90R46-R50) ```diff if [ $1 -eq 1 ]; then + # On initial install, enable and start the service systemctl enable --now serviceradar-agent.service >/dev/null 2>&1 || : -else - systemctl try-restart serviceradar-agent.service >/dev/null 2>&1 || : fi ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=2 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that using `try-restart` in the `%post` scriptlet is redundant if `%systemd_postun_with_restart` is used for upgrades, which is a standard packaging practice. Removing the `else` block simplifies the script and avoids a potential double-restart on upgrades. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More <!-- /improve --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
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
carverauto/serviceradar!2251
No description provided.