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

RPM native build updates #2253

Merged
mfreeman451 merged 8 commits from refs/pull/2253/head into main 2025-09-30 02:10:49 +00:00
Conversation 2 Commits 8 Files changed 33 +2076 -732
mfreeman451 commented 2025-09-29 19:29:57 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #1674
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1674
Original created: 2025-09-29T19:29:57Z
Original updated: 2025-09-30T02:12:46Z
Original head: carverauto/serviceradar:bug/core_rpm_installer_homedir
Original base: main
Original merged: 2025-09-30T02:10:49Z by @mfreeman451

PR Type

Enhancement

Description

  • Switch to native RPM builds on Oracle Linux/RHEL

  • Standardize user management with managed home directories

  • Update packaging file paths for consistency

  • Add dedicated web component build script

Diagram Walkthrough

flowchart LR
  A["Docker builds"] --> B["Native RPM builds"]
  B --> C["Standardized user creation"]
  C --> D["Managed home directories"]
  E["Inconsistent paths"] --> F["Unified packaging paths"]

File Walkthrough
Relevant files
Enhancement
22 files
build-web-rpm.sh
Add native web RPM build script                                                   
+132/-0 
setup-package.sh
Switch to native RPM builds                                                           
+100/-15
serviceradar-agent.spec
Standardize user management and paths                                       
+18/-7   
serviceradar-cli.spec
Update user creation with home directory                                 
+14/-2   
serviceradar-core.spec
Add dependencies and logging configuration                             
+69/-8   
serviceradar-event-writer.spec
Update paths and user management                                                 
+16/-4   
serviceradar-flowgger.spec
Update paths and user creation                                                     
+16/-4   
serviceradar-goflow2.spec
Standardize user management approach                                         
+13/-5   
serviceradar-kv.spec
Update paths and user creation                                                     
+16/-4   
serviceradar-mapper.spec
Update paths and user creation                                                     
+16/-4   
serviceradar-otel.spec
Update paths and user creation                                                     
+15/-3   
serviceradar-poller.spec
Update user management and paths                                                 
+17/-6   
serviceradar-profiler.spec
Update paths and user creation                                                     
+15/-3   
serviceradar-rperf-checker.spec
Update paths and user creation                                                     
+16/-6   
serviceradar-rperf.spec
Update paths and user creation                                                     
+15/-5   
serviceradar-snmp-checker.spec
Update paths and user creation                                                     
+17/-7   
serviceradar-srql.spec
Update paths and user creation                                                     
+13/-3   
serviceradar-sync.spec
Update paths and user creation                                                     
+16/-4   
serviceradar-sysmon.spec
Update paths and user creation                                                     
+17/-7   
serviceradar-trapd.spec
Update paths and user creation                                                     
+16/-4   
serviceradar-web.spec
Update paths and user creation                                                     
+18/-6   
serviceradar-zen.spec
Update paths and user creation                                                     
+17/-5   
Configuration changes
3 files
serviceradar-faker.spec
Fix packaging file paths                                                                 
+2/-2     
serviceradar-nats.spec
Fix packaging file paths                                                                 
+2/-2     
serviceradar-proton.spec
Fix packaging file paths                                                                 
+4/-4     
Imported from GitHub pull request. Original GitHub pull request: #1674 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1674 Original created: 2025-09-29T19:29:57Z Original updated: 2025-09-30T02:12:46Z Original head: carverauto/serviceradar:bug/core_rpm_installer_homedir Original base: main Original merged: 2025-09-30T02:10:49Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Switch to native RPM builds on Oracle Linux/RHEL - Standardize user management with managed home directories - Update packaging file paths for consistency - Add dedicated web component build script ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Docker builds"] --> B["Native RPM builds"] B --> C["Standardized user creation"] C --> D["Managed home directories"] E["Inconsistent paths"] --> F["Unified packaging paths"] ``` <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>build-web-rpm.sh</strong><dd><code>Add native web RPM build script</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-9750bd39fd69a4d0a445cd1c23a4199b41c6dd388cafcb0a1a7fd5081a5d6083">+132/-0</a>&nbsp; </td> </tr> <tr> <td><strong>setup-package.sh</strong><dd><code>Switch to native RPM builds</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; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-388e4f6f99131c27ccade0a4d7f16112297ec05e4dda0a23e142f787498ac004">+100/-15</a></td> </tr> <tr> <td><strong>serviceradar-agent.spec</strong><dd><code>Standardize user management and paths</code>&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/1674/files#diff-9fd8b118e9eb1b76ed4cc7664b788115634d3f1978bcc4fee0ea5873de1f6b90">+18/-7</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-cli.spec</strong><dd><code>Update user creation with home directory</code>&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/1674/files#diff-3ef7145d733507dc5d259973f68cac9adf0e7db60f2b96d77f6ecffb8dad93cd">+14/-2</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-core.spec</strong><dd><code>Add dependencies and logging configuration</code>&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/1674/files#diff-b03570dbdd332568f77902ae5a6dc38255e2639e1fa4bb921c06c1557dbf35e0">+69/-8</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-event-writer.spec</strong><dd><code>Update paths and user management</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-2a02c3cc8b4a3ab0ba76ce10f8c2e9168179e0f9f3eaa19807bb77d9a79b30e9">+16/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-flowgger.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-894a88a48d87c8564c8958dff8a5d186cff83a1b48163182436f7f324f765e8c">+16/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-goflow2.spec</strong><dd><code>Standardize user management approach</code>&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/1674/files#diff-bab457844021064fedca41f57b7eca08a506f4985a9364da6e1691b7167aebd9">+13/-5</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-kv.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-413b49672fc2bf028e0db1cf08f88fdc57608a82e4dfe3aed2cec699d3237d75">+16/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-mapper.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-169bda1e25390e292d863fec1a7e455de7800143e08312ff3f67349b5edd0270">+16/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-otel.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-593ca55958a2e0326e3eadce02fdd31e240e68bdf31fda39b9607d0c23cdf6c4">+15/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-poller.spec</strong><dd><code>Update user management and paths</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-afe5af03df8123f1126cf5b790cca19c707968dae11d300dd75a52097323857f">+17/-6</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-profiler.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-268b89d5448fb107cdfe16cc0d9964781c02dc39dc828725315f482fd48e16b6">+15/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-rperf-checker.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-fd3217cc52aeca1a10d4826f4b78fd2319a39a11039d798840baab1d0d174875">+16/-6</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-rperf.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-e29a7a6d3f0619a2460280c62bbd7bb8f608fde56c8078041c42cbe41e9ad95e">+15/-5</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-snmp-checker.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-b29e95ee68a3151965ee19d69ed21f519ddf530dde9dfd1ad8d90e88b48717c7">+17/-7</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-srql.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-0f0d2551b721585ca16456a6217f4633c70f56c4a667e721d6783744ca3e5ff8">+13/-3</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-sync.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-ad03df822d4f446590c2e7f35f39921575ea7f7600209eabd02ea4888fd3f8d9">+16/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-sysmon.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-1140c1c7039cda75a719ccf637cec7bb67e49313ebcbe632f34381901c5c5005">+17/-7</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-trapd.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-230b82e75a46737b86cede13d2b7c9a8b63e11f5ecffd8593d37ca8d637ff65e">+16/-4</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-web.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-c84b985ecad45b6ee62657446e763878a6dd4e9a01e5903053cc923d88c34edb">+18/-6</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-zen.spec</strong><dd><code>Update paths and user creation</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; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-b09626493d21438651625c21e3a01752c29aff928093ae915b0f738ae1e4404c">+17/-5</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>serviceradar-faker.spec</strong><dd><code>Fix packaging file paths</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; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-b67628b7eed526cebc375a650e668a829275a30b864e05bce901cb19081e8cdb">+2/-2</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-nats.spec</strong><dd><code>Fix packaging file paths</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; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-3341a04774c8bd3113142b3abaab5c5f4db8e12cc6a6a789b30555f602444172">+2/-2</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>serviceradar-proton.spec</strong><dd><code>Fix packaging file paths</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; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-6aa5dc112bfc6b27d076f2a6a5f87cf7a285ec70dbf032f720b9b4c9dc646eab">+4/-4</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-09-29 19:30:38 +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/1674#issuecomment-3348727914
Original created: 2025-09-29T19:30:38Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

Privilege escalation and capabilities:
Multiple spec %post scripts and build scripts use sudo, setcap, and create system users/dirs. Ensure setcap usage in serviceradar-agent (%post) grants only necessary capabilities and handles absence of sudo (RPM scripts run as root; sudo is not needed and may fail). Also confirm permissions on /var/lib/serviceradar (0750) and log dirs prevent unauthorized access while allowing required service read/write.

⚡ Recommended focus areas for review

Robustness

Several shell commands assume sudo, network access, and writable HOME without error handling (e.g., dnf installs, rpmdev-setuptree, npm install/build). Consider explicit checks/fail-fast messages, and handling non-interactive sudo or missing permissions. Also, writing build-info.json assumes public dir exists.

 
# Ensure required packages are installed
echo "Checking dependencies..."
if ! command -v rpmbuild &> /dev/null; then
    echo "Installing RPM build tools..."
    sudo dnf install -y rpm-build rpmdevtools
fi

if ! command -v node &> /dev/null; then
    echo "Installing Node.js 20..."
    sudo dnf module enable -y nodejs:20
    sudo dnf install -y nodejs
fi

NODE_VERSION=$(node --version)
echo "Using Node.js $NODE_VERSION"

# Set up RPM build environment
echo "Setting up RPM build environment..."
rpmdev-setuptree

# Create necessary directories
mkdir -p "${RPMBUILD_DIR}/SOURCES/systemd"
mkdir -p "${RPMBUILD_DIR}/SOURCES/config"
mkdir -p "${RPMBUILD_DIR}/SOURCES/selinux"
mkdir -p "${RPMBUILD_DIR}/BUILD/web"

# Build Next.js application
echo "Building Next.js application..."
cd "${BASE_DIR}/web"

# Clean install
echo "Installing dependencies..."
npm cache clean --force
npm install --omit=optional

# Build with version info
echo "Building Next.js with standalone output..."
NEXT_PUBLIC_VERSION="$VERSION" NEXT_PUBLIC_BUILD_ID="$BUILD_ID" npm run build

# Verify standalone output exists
if [ ! -d ".next/standalone" ]; then
    echo "ERROR: .next/standalone not found. Make sure next.config.js has output: 'standalone'"
    exit 1
fi

# Copy to RPM build directory
echo "Copying built files to RPM build directory..."
cp -r .next/standalone/* "${RPMBUILD_DIR}/BUILD/web/"
mkdir -p "${RPMBUILD_DIR}/BUILD/web/.next"
cp -r .next/static "${RPMBUILD_DIR}/BUILD/web/.next/"
cp -r .next/standalone/.next/* "${RPMBUILD_DIR}/BUILD/web/.next/" 2>/dev/null || true

# Copy public directory if it exists
if [ -d "public" ]; then
    cp -r public "${RPMBUILD_DIR}/BUILD/web/"
fi

# Create build-info.json
cat > "${RPMBUILD_DIR}/BUILD/web/public/build-info.json" << EOF
{
  "version": "$VERSION",
  "buildId": "$BUILD_ID",
  "buildTime": "$(date -u +"%Y-%m-%dT%H:%M:%SZ")"
}
EOF
OS Detection

Grepping /etc/os-release for multiple distros may mis-detect or miss variants; rely on ID/ID_LIKE keys for more reliable native RPM build gating. Validate behavior on non-RHEL-like systems.

 
# Detect if we're on Oracle Linux/RHEL/Rocky - if so, build natively
if grep -qE "Oracle Linux|Red Hat|Rocky Linux|AlmaLinux" /etc/os-release 2>/dev/null; then
    echo "Building RPM natively on $(grep PRETTY_NAME /etc/os-release | cut -d'"' -f2)..."

    # Special handling for web component - use dedicated build script
    if [ "$component" = "web" ]; then
        export VERSION="$version"
        export BUILD_ID="$BUILD_ID"
        export RELEASE="$rpm_release"
        "${BASE_DIR}/scripts/build-web-rpm.sh" || { echo "Error: Native RPM build failed"; exit 1; }
        echo "RPM built: ${RELEASE_DIR}/rpm/${version}/${package_name}-${version}-${rpm_release}.*.rpm"
    else
        # Native RPM build for all other components
        echo "Building ${component} natively..."

        # Ensure RPM build tools are installed
        if ! command -v rpmbuild &> /dev/null; then
            echo "Installing rpmbuild..."
            sudo dnf install -y rpm-build rpmdevtools
        fi

        # Set up RPM build directory
        RPMBUILD_DIR="${HOME}/rpmbuild"
        rpmdev-setuptree

        # Build binary if needed
        if [ "$build_method" = "go" ] && [ -n "$src_path" ]; then
            echo "Building Go binary from $src_path..."
            output_path=$(echo "$config" | jq -r '.binary.output_path')
            GOOS=linux GOARCH=amd64 go build \
                -ldflags "-X github.com/carverauto/serviceradar/pkg/version.version=$version -X github.com/carverauto/serviceradar/pkg/version.buildID=$BUILD_ID" \
                -o "${RPMBUILD_DIR}/BUILD/$(basename $output_path)" \
                "${BASE_DIR}/${src_path}" || { echo "Error: Go build failed"; exit 1; }
        elif [ "$build_method" = "none" ]; then
            echo "No binary build required for $component"
        else
            echo "Build method '$build_method' not yet supported for native RPM builds - falling back to Docker"
            # Fall back to Docker for unsupported build methods
            if [ -n "$dockerfile" ]; then
                docker build \
                    --platform linux/amd64 \
                    --build-arg VERSION="$version" \
                    --build-arg BUILD_ID="$BUILD_ID" \
                    --build-arg RELEASE="$rpm_release" \
                    --build-arg COMPONENT="$component" \
                    --build-arg BINARY_PATH="$src_path" \
                    -f "${BASE_DIR}/${dockerfile}" \
                    -t "${package_name}-rpm-builder" \
                    "${BASE_DIR}" || { echo "Error: Docker build failed"; exit 1; }
                tmp_dir=$(mktemp -d)
                container_id=$(docker create "${package_name}-rpm-builder" /bin/true)
                docker cp "$container_id:/rpms/." "$tmp_dir/"
                mkdir -p "${RELEASE_DIR}/rpm/${version}"
                find "$tmp_dir" -name "*.rpm" -exec cp {} "${RELEASE_DIR}/rpm/${version}/" \;
                docker rm "$container_id"
                rm -rf "$tmp_dir"
                continue
            fi
        fi

        # Copy source files needed for RPM build
        mkdir -p "${RPMBUILD_DIR}/SOURCES"

        # Copy the entire packaging directory for this component
        # This ensures all config files, scripts, systemd services, etc. are available
        if [ -d "${BASE_DIR}/packaging/${component}" ]; then
            echo "Copying packaging/${component} to SOURCES..."
            mkdir -p "${RPMBUILD_DIR}/SOURCES/packaging"
            cp -r "${BASE_DIR}/packaging/${component}" "${RPMBUILD_DIR}/SOURCES/packaging/"
        fi

        # Copy spec file
        spec_file="${BASE_DIR}/packaging/specs/${package_name}.spec"
        if [ ! -f "$spec_file" ]; then
            echo "Error: Spec file not found: $spec_file"
            exit 1
        fi
        cp "$spec_file" "${RPMBUILD_DIR}/SPECS/"

        # Build RPM
        RPM_VERSION=$(echo ${version} | sed 's/-/_/g')
        echo "Building RPM package with version ${RPM_VERSION}..."
        rpmbuild -bb \
            --define "version ${RPM_VERSION}" \
            --define "release ${rpm_release}" \
            --define "_sourcedir ${RPMBUILD_DIR}/SOURCES" \
            --define "_builddir ${RPMBUILD_DIR}/BUILD" \
            --define "_rpmdir ${RPMBUILD_DIR}/RPMS" \
            --nocheck \
            "${RPMBUILD_DIR}/SPECS/${package_name}.spec" || { echo "Error: rpmbuild failed"; exit 1; }

        # Copy RPM to release directory
        mkdir -p "${RELEASE_DIR}/rpm/${version}"
        find "${RPMBUILD_DIR}/RPMS" -name "*.rpm" -exec cp {} "${RELEASE_DIR}/rpm/${version}/" \;
        echo "RPM built: ${RELEASE_DIR}/rpm/${version}/${package_name}-${RPM_VERSION}-${rpm_release}.*.rpm"
    fi
elif [ -n "$dockerfile" ]; then
Post Script Changes

The %post section now creates directories and runs an inline Python script to mutate config. Verify python3 is present at runtime, SELinux/file contexts, and that config edits don't overwrite admin changes unintentionally.

 
if systemctl is-active --quiet firewalld; then
    firewall-cmd --permanent --add-port=8090/tcp --zone=public
    firewall-cmd --permanent --add-port=50052/tcp --zone=public
    firewall-cmd --reload
    logger "Configured firewalld for ServiceRadar (ports 8090/tcp and 50052/tcp, zone public)."
fi

# Create data directory with proper permissions
install -d -m 0750 -o serviceradar -g serviceradar /var/lib/serviceradar

# Ensure log directory exists for configured log files
install -d -m 0755 -o serviceradar -g serviceradar /var/log/serviceradar

# Ensure user cache directory exists to satisfy goimports modindex cache
install -d -m 0700 -o serviceradar -g serviceradar /var/lib/serviceradar/.cache

# Set proper permissions for configuration
chown -R serviceradar:serviceradar /etc/serviceradar
chmod -R 755 /etc/serviceradar

# Ensure logging configuration exists for upgraded installations
if [ -f /etc/serviceradar/core.json ]; then
    /usr/bin/python3 <<'PY'
import json
import os

path = "/etc/serviceradar/core.json"

try:
    with open(path, "r", encoding="utf-8") as handle:
        data = json.load(handle)
except Exception:
    data = None

if isinstance(data, dict):
    logging_cfg = data.get("logging")
    if not isinstance(logging_cfg, dict) or not logging_cfg.get("level"):
        data["logging"] = {
            "level": "info",
            "debug": False,
            "output": "stdout",
            "time_format": "",
            "otel": {
                "enabled": False,
                "endpoint": "",
                "service_name": "serviceradar-core",
                "batch_timeout": "5s",
                "insecure": False,
                "headers": {}
            }
        }
        tmp_path = f"{path}.rpmtmp"
        with open(tmp_path, "w", encoding="utf-8") as handle:
            json.dump(data, handle, indent=4)
            handle.write("\n")
        os.replace(tmp_path, path)
PY
    chown serviceradar:serviceradar /etc/serviceradar/core.json
    chmod 0644 /etc/serviceradar/core.json
fi

# Start and enable service
systemctl daemon-reload
systemctl enable serviceradar-core
systemctl start serviceradar-core || echo "Failed to start service, please check the logs with: journalctl -xeu serviceradar-core"
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1674#issuecomment-3348727914 Original created: 2025-09-29T19:30:38Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️&nbsp;<strong>Estimated effort to review</strong>: 3 🔵🔵🔵⚪⚪</td></tr> <tr><td>🧪&nbsp;<strong>No relevant tests</strong></td></tr> <tr><td>🔒&nbsp;<strong>Security concerns</strong><br><br> <strong>Privilege escalation and capabilities:</strong><br> Multiple spec %post scripts and build scripts use sudo, setcap, and create system users/dirs. Ensure setcap usage in serviceradar-agent (%post) grants only necessary capabilities and handles absence of sudo (RPM scripts run as root; sudo is not needed and may fail). Also confirm permissions on /var/lib/serviceradar (0750) and log dirs prevent unauthorized access while allowing required service read/write.</td></tr> <tr><td>⚡&nbsp;<strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1674/files#diff-9750bd39fd69a4d0a445cd1c23a4199b41c6dd388cafcb0a1a7fd5081a5d6083R33-R98'><strong>Robustness</strong></a> Several shell commands assume sudo, network access, and writable HOME without error handling (e.g., dnf installs, rpmdev-setuptree, npm install/build). Consider explicit checks/fail-fast messages, and handling non-interactive sudo or missing permissions. Also, writing build-info.json assumes public dir exists. </summary> ```shell # Ensure required packages are installed echo "Checking dependencies..." if ! command -v rpmbuild &> /dev/null; then echo "Installing RPM build tools..." sudo dnf install -y rpm-build rpmdevtools fi if ! command -v node &> /dev/null; then echo "Installing Node.js 20..." sudo dnf module enable -y nodejs:20 sudo dnf install -y nodejs fi NODE_VERSION=$(node --version) echo "Using Node.js $NODE_VERSION" # Set up RPM build environment echo "Setting up RPM build environment..." rpmdev-setuptree # Create necessary directories mkdir -p "${RPMBUILD_DIR}/SOURCES/systemd" mkdir -p "${RPMBUILD_DIR}/SOURCES/config" mkdir -p "${RPMBUILD_DIR}/SOURCES/selinux" mkdir -p "${RPMBUILD_DIR}/BUILD/web" # Build Next.js application echo "Building Next.js application..." cd "${BASE_DIR}/web" # Clean install echo "Installing dependencies..." npm cache clean --force npm install --omit=optional # Build with version info echo "Building Next.js with standalone output..." NEXT_PUBLIC_VERSION="$VERSION" NEXT_PUBLIC_BUILD_ID="$BUILD_ID" npm run build # Verify standalone output exists if [ ! -d ".next/standalone" ]; then echo "ERROR: .next/standalone not found. Make sure next.config.js has output: 'standalone'" exit 1 fi # Copy to RPM build directory echo "Copying built files to RPM build directory..." cp -r .next/standalone/* "${RPMBUILD_DIR}/BUILD/web/" mkdir -p "${RPMBUILD_DIR}/BUILD/web/.next" cp -r .next/static "${RPMBUILD_DIR}/BUILD/web/.next/" cp -r .next/standalone/.next/* "${RPMBUILD_DIR}/BUILD/web/.next/" 2>/dev/null || true # Copy public directory if it exists if [ -d "public" ]; then cp -r public "${RPMBUILD_DIR}/BUILD/web/" fi # Create build-info.json cat > "${RPMBUILD_DIR}/BUILD/web/public/build-info.json" << EOF { "version": "$VERSION", "buildId": "$BUILD_ID", "buildTime": "$(date -u +"%Y-%m-%dT%H:%M:%SZ")" } EOF ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1674/files#diff-388e4f6f99131c27ccade0a4d7f16112297ec05e4dda0a23e142f787498ac004R525-R621'><strong>OS Detection</strong></a> Grepping /etc/os-release for multiple distros may mis-detect or miss variants; rely on ID/ID_LIKE keys for more reliable native RPM build gating. Validate behavior on non-RHEL-like systems. </summary> ```shell # Detect if we're on Oracle Linux/RHEL/Rocky - if so, build natively if grep -qE "Oracle Linux|Red Hat|Rocky Linux|AlmaLinux" /etc/os-release 2>/dev/null; then echo "Building RPM natively on $(grep PRETTY_NAME /etc/os-release | cut -d'"' -f2)..." # Special handling for web component - use dedicated build script if [ "$component" = "web" ]; then export VERSION="$version" export BUILD_ID="$BUILD_ID" export RELEASE="$rpm_release" "${BASE_DIR}/scripts/build-web-rpm.sh" || { echo "Error: Native RPM build failed"; exit 1; } echo "RPM built: ${RELEASE_DIR}/rpm/${version}/${package_name}-${version}-${rpm_release}.*.rpm" else # Native RPM build for all other components echo "Building ${component} natively..." # Ensure RPM build tools are installed if ! command -v rpmbuild &> /dev/null; then echo "Installing rpmbuild..." sudo dnf install -y rpm-build rpmdevtools fi # Set up RPM build directory RPMBUILD_DIR="${HOME}/rpmbuild" rpmdev-setuptree # Build binary if needed if [ "$build_method" = "go" ] && [ -n "$src_path" ]; then echo "Building Go binary from $src_path..." output_path=$(echo "$config" | jq -r '.binary.output_path') GOOS=linux GOARCH=amd64 go build \ -ldflags "-X github.com/carverauto/serviceradar/pkg/version.version=$version -X github.com/carverauto/serviceradar/pkg/version.buildID=$BUILD_ID" \ -o "${RPMBUILD_DIR}/BUILD/$(basename $output_path)" \ "${BASE_DIR}/${src_path}" || { echo "Error: Go build failed"; exit 1; } elif [ "$build_method" = "none" ]; then echo "No binary build required for $component" else echo "Build method '$build_method' not yet supported for native RPM builds - falling back to Docker" # Fall back to Docker for unsupported build methods if [ -n "$dockerfile" ]; then docker build \ --platform linux/amd64 \ --build-arg VERSION="$version" \ --build-arg BUILD_ID="$BUILD_ID" \ --build-arg RELEASE="$rpm_release" \ --build-arg COMPONENT="$component" \ --build-arg BINARY_PATH="$src_path" \ -f "${BASE_DIR}/${dockerfile}" \ -t "${package_name}-rpm-builder" \ "${BASE_DIR}" || { echo "Error: Docker build failed"; exit 1; } tmp_dir=$(mktemp -d) container_id=$(docker create "${package_name}-rpm-builder" /bin/true) docker cp "$container_id:/rpms/." "$tmp_dir/" mkdir -p "${RELEASE_DIR}/rpm/${version}" find "$tmp_dir" -name "*.rpm" -exec cp {} "${RELEASE_DIR}/rpm/${version}/" \; docker rm "$container_id" rm -rf "$tmp_dir" continue fi fi # Copy source files needed for RPM build mkdir -p "${RPMBUILD_DIR}/SOURCES" # Copy the entire packaging directory for this component # This ensures all config files, scripts, systemd services, etc. are available if [ -d "${BASE_DIR}/packaging/${component}" ]; then echo "Copying packaging/${component} to SOURCES..." mkdir -p "${RPMBUILD_DIR}/SOURCES/packaging" cp -r "${BASE_DIR}/packaging/${component}" "${RPMBUILD_DIR}/SOURCES/packaging/" fi # Copy spec file spec_file="${BASE_DIR}/packaging/specs/${package_name}.spec" if [ ! -f "$spec_file" ]; then echo "Error: Spec file not found: $spec_file" exit 1 fi cp "$spec_file" "${RPMBUILD_DIR}/SPECS/" # Build RPM RPM_VERSION=$(echo ${version} | sed 's/-/_/g') echo "Building RPM package with version ${RPM_VERSION}..." rpmbuild -bb \ --define "version ${RPM_VERSION}" \ --define "release ${rpm_release}" \ --define "_sourcedir ${RPMBUILD_DIR}/SOURCES" \ --define "_builddir ${RPMBUILD_DIR}/BUILD" \ --define "_rpmdir ${RPMBUILD_DIR}/RPMS" \ --nocheck \ "${RPMBUILD_DIR}/SPECS/${package_name}.spec" || { echo "Error: rpmbuild failed"; exit 1; } # Copy RPM to release directory mkdir -p "${RELEASE_DIR}/rpm/${version}" find "${RPMBUILD_DIR}/RPMS" -name "*.rpm" -exec cp {} "${RELEASE_DIR}/rpm/${version}/" \; echo "RPM built: ${RELEASE_DIR}/rpm/${version}/${package_name}-${RPM_VERSION}-${rpm_release}.*.rpm" fi elif [ -n "$dockerfile" ]; then ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1674/files#diff-b03570dbdd332568f77902ae5a6dc38255e2639e1fa4bb921c06c1557dbf35e0R154-R218'><strong>Post Script Changes</strong></a> The %post section now creates directories and runs an inline Python script to mutate config. Verify python3 is present at runtime, SELinux/file contexts, and that config edits don't overwrite admin changes unintentionally. </summary> ```txt if systemctl is-active --quiet firewalld; then firewall-cmd --permanent --add-port=8090/tcp --zone=public firewall-cmd --permanent --add-port=50052/tcp --zone=public firewall-cmd --reload logger "Configured firewalld for ServiceRadar (ports 8090/tcp and 50052/tcp, zone public)." fi # Create data directory with proper permissions install -d -m 0750 -o serviceradar -g serviceradar /var/lib/serviceradar # Ensure log directory exists for configured log files install -d -m 0755 -o serviceradar -g serviceradar /var/log/serviceradar # Ensure user cache directory exists to satisfy goimports modindex cache install -d -m 0700 -o serviceradar -g serviceradar /var/lib/serviceradar/.cache # Set proper permissions for configuration chown -R serviceradar:serviceradar /etc/serviceradar chmod -R 755 /etc/serviceradar # Ensure logging configuration exists for upgraded installations if [ -f /etc/serviceradar/core.json ]; then /usr/bin/python3 <<'PY' import json import os path = "/etc/serviceradar/core.json" try: with open(path, "r", encoding="utf-8") as handle: data = json.load(handle) except Exception: data = None if isinstance(data, dict): logging_cfg = data.get("logging") if not isinstance(logging_cfg, dict) or not logging_cfg.get("level"): data["logging"] = { "level": "info", "debug": False, "output": "stdout", "time_format": "", "otel": { "enabled": False, "endpoint": "", "service_name": "serviceradar-core", "batch_timeout": "5s", "insecure": False, "headers": {} } } tmp_path = f"{path}.rpmtmp" with open(tmp_path, "w", encoding="utf-8") as handle: json.dump(data, handle, indent=4) handle.write("\n") os.replace(tmp_path, path) PY chown serviceradar:serviceradar /etc/serviceradar/core.json chmod 0644 /etc/serviceradar/core.json fi # Start and enable service systemctl daemon-reload systemctl enable serviceradar-core systemctl start serviceradar-core || echo "Failed to start service, please check the logs with: journalctl -xeu serviceradar-core" ``` </details> </td></tr> </table>
qodo-code-review[bot] commented 2025-09-29 19:32:28 +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/1674#issuecomment-3348735131
Original created: 2025-09-29T19:32:28Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consolidate user management into a common package

The user and group creation logic for 'serviceradar' is duplicated across
numerous RPM spec files. This should be consolidated into a single, common base
package that other packages can depend on to improve maintainability.

Examples:

packaging/specs/serviceradar-agent.spec [36-52] 
%pre
# Ensure serviceradar group exists before user creation
if ! getent group serviceradar >/dev/null; then
    groupadd --system serviceradar
fi

# Create serviceradar user with managed home directory if it doesn't exist
if ! id -u serviceradar >/dev/null 2>&1; then
    useradd --system --home-dir /var/lib/serviceradar --create-home \
        --shell /usr/sbin/nologin --gid serviceradar serviceradar

 ... (clipped 7 lines)
packaging/specs/serviceradar-core.spec [54-70] 
%pre
# Ensure serviceradar group exists before user creation
if ! getent group serviceradar >/dev/null; then
    groupadd --system serviceradar
fi

# Create serviceradar user with managed home directory if it doesn't exist
if ! id -u serviceradar >/dev/null 2>&1; then
    useradd --system --home-dir /var/lib/serviceradar --create-home \
        --shell /usr/sbin/nologin --gid serviceradar serviceradar

 ... (clipped 7 lines)

Solution Walkthrough:

Before:

# In serviceradar-agent.spec, serviceradar-core.spec, etc. (repeated >15 times)
%pre
# Ensure serviceradar group exists
if ! getent group serviceradar >/dev/null; then
    groupadd --system serviceradar
fi

# Create serviceradar user if it doesn't exist
if ! id -u serviceradar >/dev/null 2>&1; then
    useradd --system --home-dir /var/lib/serviceradar --create-home \
        --shell /usr/sbin/nologin --gid serviceradar serviceradar
else
    # Align existing user home directory if needed
    ...
fi

After:

# In a new file: serviceradar-common.spec
Name:           serviceradar-common
...
%pre
# Ensure serviceradar group exists
if ! getent group serviceradar >/dev/null; then
    groupadd --system serviceradar
fi
# Create serviceradar user if it doesn't exist
if ! id -u serviceradar >/dev/null 2>&1; then
    useradd --system --home-dir /var/lib/serviceradar --create-home ...
else
    # Align existing user home directory if needed
    ...
fi

# In serviceradar-agent.spec, serviceradar-core.spec, etc.
Requires:       serviceradar-common
...
# The entire %pre section for user creation is removed.
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies widespread logic duplication for user creation across many RPM spec files and proposes a standard, more maintainable architectural solution.

Medium
Possible issue
Add proper error handling

Improve the embedded Python script by adding specific error handling for file
I/O and JSON parsing. This will prevent silent failures and ensure the system
remains in a consistent state.

packaging/specs/serviceradar-core.spec [176-210]

 /usr/bin/python3 <<'PY'
 import json
 import os
+import sys
 
 path = "/etc/serviceradar/core.json"
 
 try:
     with open(path, "r", encoding="utf-8") as handle:
         data = json.load(handle)
-except Exception:
-    data = None
+except (FileNotFoundError, PermissionError, json.JSONDecodeError) as e:
+    print(f"Warning: Could not read {path}: {e}", file=sys.stderr)
+    sys.exit(0)
 
 if isinstance(data, dict):
     logging_cfg = data.get("logging")
     if not isinstance(logging_cfg, dict) or not logging_cfg.get("level"):
         data["logging"] = {
             "level": "info",
             "debug": False,
             "output": "stdout",
             "time_format": "",
             "otel": {
                 "enabled": False,
                 "endpoint": "",
                 "service_name": "serviceradar-core",
                 "batch_timeout": "5s",
                 "insecure": False,
                 "headers": {}
             }
         }
         tmp_path = f"{path}.rpmtmp"
-        with open(tmp_path, "w", encoding="utf-8") as handle:
-            json.dump(data, handle, indent=4)
-            handle.write("\n")
-        os.replace(tmp_path, path)
+        try:
+            with open(tmp_path, "w", encoding="utf-8") as handle:
+                json.dump(data, handle, indent=4)
+                handle.write("\n")
+            os.replace(tmp_path, path)
+        except (PermissionError, OSError) as e:
+            print(f"Error: Could not update {path}: {e}", file=sys.stderr)
+            if os.path.exists(tmp_path):
+                os.unlink(tmp_path)
+            sys.exit(1)
 PY
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a lack of robust error handling in the embedded Python script, which could cause silent failures during package installation. The proposed changes make the script more resilient and provide better diagnostics, which is important for package management scripts.

Medium
General
Add package manager detection

Make the script compatible with more systems by checking for dnf and falling
back to yum if dnf is not available. This avoids failures on older distributions
that use yum.

scripts/setup-package.sh [543]

-sudo dnf install -y rpm-build rpmdevtools
+if command -v dnf &> /dev/null; then
+    sudo dnf install -y rpm-build rpmdevtools
+elif command -v yum &> /dev/null; then
+    sudo yum install -y rpm-build rpmdevtools
+else
+    echo "Error: Neither dnf nor yum package manager found"
+    exit 1
+fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that hardcoding dnf limits the script's compatibility with older RHEL/CentOS systems that use yum. Adding a fallback to yum improves the script's portability and robustness.

Low
Add package manager compatibility

Improve package manager compatibility by checking for dnf and falling back to
yum if it's not present. This allows the script to run on a wider range of
RHEL-based distributions.

scripts/build-web-rpm.sh [35-44]

 if ! command -v rpmbuild &> /dev/null; then
     echo "Installing RPM build tools..."
-    sudo dnf install -y rpm-build rpmdevtools
+    if command -v dnf &> /dev/null; then
+        sudo dnf install -y rpm-build rpmdevtools
+    elif command -v yum &> /dev/null; then
+        sudo yum install -y rpm-build rpmdevtools
+    else
+        echo "Error: Neither dnf nor yum package manager found"
+        exit 1
+    fi
 fi
 
 if ! command -v node &> /dev/null; then
     echo "Installing Node.js 20..."
-    sudo dnf module enable -y nodejs:20
-    sudo dnf install -y nodejs
+    if command -v dnf &> /dev/null; then
+        sudo dnf module enable -y nodejs:20
+        sudo dnf install -y nodejs
+    elif command -v yum &> /dev/null; then
+        sudo yum install -y nodejs
+    else
+        echo "Error: Cannot install Node.js - no supported package manager found"
+        exit 1
+    fi
 fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that hardcoding dnf limits the script's portability to RHEL/CentOS versions that use yum. The proposed change to support both dnf and yum makes the build script more robust and compatible across different environments.

Low
Optimize OS detection logic

Refactor the OS detection logic to avoid calling grep twice on /etc/os-release.
Add a fallback to handle cases where PRETTY_NAME is not found.

scripts/setup-package.sh [526-527]

-if grep -qE "Oracle Linux|Red Hat|Rocky Linux|AlmaLinux" /etc/os-release 2>/dev/null; then
-    echo "Building RPM natively on $(grep PRETTY_NAME /etc/os-release | cut -d'"' -f2)..."
+if os_info=$(grep -E "Oracle Linux|Red Hat|Rocky Linux|AlmaLinux" /etc/os-release 2>/dev/null); then
+    pretty_name=$(grep PRETTY_NAME /etc/os-release 2>/dev/null | cut -d'"' -f2 || echo "Unknown Linux")
+    echo "Building RPM natively on $pretty_name..."
  • Apply / Chat
Suggestion importance[1-10]: 2

__

Why: The suggestion's primary claim of inefficiency is negligible, and the proposed code still calls grep twice. The only minor improvement is adding a fallback for PRETTY_NAME, which slightly increases robustness but does not address the stated problem.

Low
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1674#issuecomment-3348735131 Original created: 2025-09-29T19:32:28Z --- ## PR Code Suggestions ✨ <!-- dbe5fe6 --> 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>Consolidate user management into a common package</summary> ___ **The user and group creation logic for 'serviceradar' is duplicated across <br>numerous RPM spec files. This should be consolidated into a single, common base <br>package that other packages can depend on to improve maintainability.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-9fd8b118e9eb1b76ed4cc7664b788115634d3f1978bcc4fee0ea5873de1f6b90R36-R52">packaging/specs/serviceradar-agent.spec [36-52]</a> </summary> ```spec %pre # Ensure serviceradar group exists before user creation if ! getent group serviceradar >/dev/null; then groupadd --system serviceradar fi # Create serviceradar user with managed home directory if it doesn't exist if ! id -u serviceradar >/dev/null 2>&1; then useradd --system --home-dir /var/lib/serviceradar --create-home \ --shell /usr/sbin/nologin --gid serviceradar serviceradar ... (clipped 7 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1674/files#diff-b03570dbdd332568f77902ae5a6dc38255e2639e1fa4bb921c06c1557dbf35e0R54-R70">packaging/specs/serviceradar-core.spec [54-70]</a> </summary> ```spec %pre # Ensure serviceradar group exists before user creation if ! getent group serviceradar >/dev/null; then groupadd --system serviceradar fi # Create serviceradar user with managed home directory if it doesn't exist if ! id -u serviceradar >/dev/null 2>&1; then useradd --system --home-dir /var/lib/serviceradar --create-home \ --shell /usr/sbin/nologin --gid serviceradar serviceradar ... (clipped 7 lines) ``` </details> ### Solution Walkthrough: #### Before: ```spec # In serviceradar-agent.spec, serviceradar-core.spec, etc. (repeated >15 times) %pre # Ensure serviceradar group exists if ! getent group serviceradar >/dev/null; then groupadd --system serviceradar fi # Create serviceradar user if it doesn't exist if ! id -u serviceradar >/dev/null 2>&1; then useradd --system --home-dir /var/lib/serviceradar --create-home \ --shell /usr/sbin/nologin --gid serviceradar serviceradar else # Align existing user home directory if needed ... fi ``` #### After: ```spec # In a new file: serviceradar-common.spec Name: serviceradar-common ... %pre # Ensure serviceradar group exists if ! getent group serviceradar >/dev/null; then groupadd --system serviceradar fi # Create serviceradar user if it doesn't exist if ! id -u serviceradar >/dev/null 2>&1; then useradd --system --home-dir /var/lib/serviceradar --create-home ... else # Align existing user home directory if needed ... fi # In serviceradar-agent.spec, serviceradar-core.spec, etc. Requires: serviceradar-common ... # The entire %pre section for user creation is removed. ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies widespread logic duplication for user creation across many RPM spec files and proposes a standard, more maintainable architectural solution. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Add proper error handling<!-- not_implemented --></summary> ___ **Improve the embedded Python script by adding specific error handling for file <br>I/O and JSON parsing. This will prevent silent failures and ensure the system <br>remains in a consistent state.** [packaging/specs/serviceradar-core.spec [176-210]](https://github.com/carverauto/serviceradar/pull/1674/files#diff-b03570dbdd332568f77902ae5a6dc38255e2639e1fa4bb921c06c1557dbf35e0R176-R210) ```diff /usr/bin/python3 <<'PY' import json import os +import sys path = "/etc/serviceradar/core.json" try: with open(path, "r", encoding="utf-8") as handle: data = json.load(handle) -except Exception: - data = None +except (FileNotFoundError, PermissionError, json.JSONDecodeError) as e: + print(f"Warning: Could not read {path}: {e}", file=sys.stderr) + sys.exit(0) if isinstance(data, dict): logging_cfg = data.get("logging") if not isinstance(logging_cfg, dict) or not logging_cfg.get("level"): data["logging"] = { "level": "info", "debug": False, "output": "stdout", "time_format": "", "otel": { "enabled": False, "endpoint": "", "service_name": "serviceradar-core", "batch_timeout": "5s", "insecure": False, "headers": {} } } tmp_path = f"{path}.rpmtmp" - with open(tmp_path, "w", encoding="utf-8") as handle: - json.dump(data, handle, indent=4) - handle.write("\n") - os.replace(tmp_path, path) + try: + with open(tmp_path, "w", encoding="utf-8") as handle: + json.dump(data, handle, indent=4) + handle.write("\n") + os.replace(tmp_path, path) + except (PermissionError, OSError) as e: + print(f"Error: Could not update {path}: {e}", file=sys.stderr) + if os.path.exists(tmp_path): + os.unlink(tmp_path) + sys.exit(1) PY ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a lack of robust error handling in the embedded Python script, which could cause silent failures during package installation. The proposed changes make the script more resilient and provide better diagnostics, which is important for package management scripts. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=3>General</td> <td> <details><summary>Add package manager detection</summary> ___ **Make the script compatible with more systems by checking for <code>dnf</code> and falling <br>back to <code>yum</code> if <code>dnf</code> is not available. This avoids failures on older distributions <br>that use <code>yum</code>.** [scripts/setup-package.sh [543]](https://github.com/carverauto/serviceradar/pull/1674/files#diff-388e4f6f99131c27ccade0a4d7f16112297ec05e4dda0a23e142f787498ac004R543-R543) ```diff -sudo dnf install -y rpm-build rpmdevtools +if command -v dnf &> /dev/null; then + sudo dnf install -y rpm-build rpmdevtools +elif command -v yum &> /dev/null; then + sudo yum install -y rpm-build rpmdevtools +else + echo "Error: Neither dnf nor yum package manager found" + exit 1 +fi ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=2 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly points out that hardcoding `dnf` limits the script's compatibility with older RHEL/CentOS systems that use `yum`. Adding a fallback to `yum` improves the script's portability and robustness. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Add package manager compatibility</summary> ___ **Improve package manager compatibility by checking for <code>dnf</code> and falling back to <br><code>yum</code> if it's not present. This allows the script to run on a wider range of <br>RHEL-based distributions.** [scripts/build-web-rpm.sh [35-44]](https://github.com/carverauto/serviceradar/pull/1674/files#diff-9750bd39fd69a4d0a445cd1c23a4199b41c6dd388cafcb0a1a7fd5081a5d6083R35-R44) ```diff if ! command -v rpmbuild &> /dev/null; then echo "Installing RPM build tools..." - sudo dnf install -y rpm-build rpmdevtools + if command -v dnf &> /dev/null; then + sudo dnf install -y rpm-build rpmdevtools + elif command -v yum &> /dev/null; then + sudo yum install -y rpm-build rpmdevtools + else + echo "Error: Neither dnf nor yum package manager found" + exit 1 + fi fi if ! command -v node &> /dev/null; then echo "Installing Node.js 20..." - sudo dnf module enable -y nodejs:20 - sudo dnf install -y nodejs + if command -v dnf &> /dev/null; then + sudo dnf module enable -y nodejs:20 + sudo dnf install -y nodejs + elif command -v yum &> /dev/null; then + sudo yum install -y nodejs + else + echo "Error: Cannot install Node.js - no supported package manager found" + exit 1 + fi fi ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=3 --> <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that hardcoding `dnf` limits the script's portability to RHEL/CentOS versions that use `yum`. The proposed change to support both `dnf` and `yum` makes the build script more robust and compatible across different environments. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Optimize OS detection logic</summary> ___ **Refactor the OS detection logic to avoid calling <code>grep</code> twice on <code>/etc/os-release</code>. <br>Add a fallback to handle cases where <code>PRETTY_NAME</code> is not found.** [scripts/setup-package.sh [526-527]](https://github.com/carverauto/serviceradar/pull/1674/files#diff-388e4f6f99131c27ccade0a4d7f16112297ec05e4dda0a23e142f787498ac004R526-R527) ```diff -if grep -qE "Oracle Linux|Red Hat|Rocky Linux|AlmaLinux" /etc/os-release 2>/dev/null; then - echo "Building RPM natively on $(grep PRETTY_NAME /etc/os-release | cut -d'"' -f2)..." +if os_info=$(grep -E "Oracle Linux|Red Hat|Rocky Linux|AlmaLinux" /etc/os-release 2>/dev/null); then + pretty_name=$(grep PRETTY_NAME /etc/os-release 2>/dev/null | cut -d'"' -f2 || echo "Unknown Linux") + echo "Building RPM natively on $pretty_name..." ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=4 --> <details><summary>Suggestion importance[1-10]: 2</summary> __ Why: The suggestion's primary claim of inefficiency is negligible, and the proposed code still calls `grep` twice. The only minor improvement is adding a fallback for `PRETTY_NAME`, which slightly increases robustness but does not address the stated problem. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --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!2253
No description provided.