carverauto/serviceradar

Fork 0

wip #2455

Merged

mfreeman451 merged 1 commit from refs/pull/2455/head into main

2025-11-23 18:02:59 +00:00

mfreeman451 commented

2025-11-23 18:02:45 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1987
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1987
Original created: 2025-11-23T18:02:45Z
Original updated: 2025-11-23T18:03:57Z
Original head: carverauto/serviceradar:fix/rbe_rpm
Original base: main
Original merged: 2025-11-23T18:02:59Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Enhancement, Bug fix

Description

Refactor RPM packaging to support fallback to local execution
Remove hardcoded remote cache disabling step
Add retry logic for remote packaging failures
Update Bazel RPM toolchain flag to use rules_pkg namespace

Diagram Walkthrough

flowchart LR
  A["Remote packaging attempt"] -->|Success| B["Publish packages"]
  A -->|Failure| C["Local packaging fallback"]
  C --> B
  D["Remove cache disable step"] -.-> A
  E["Update toolchain flags"] -.-> A

File Walkthrough

Relevant files

Enhancement

release.yml

Implement fallback mechanism for RPM packaging

.github/workflows/release.yml

Removed the separate step that disabled remote executor/cache settings
Refactored packaging logic into two functions: run_remote() and
run_local()
Updated Bazel RPM toolchain flag from
--//toolchains/rpm:is_rpmbuild_available=1 to
--@rules_pkg//toolchains/rpm:is_rpmbuild_available=1
Added conditional retry logic to attempt local packaging if remote
execution fails

+25/-17

Imported from GitHub pull request. Original GitHub pull request: #1987 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1987 Original created: 2025-11-23T18:02:45Z Original updated: 2025-11-23T18:03:57Z Original head: carverauto/serviceradar:fix/rbe_rpm Original base: main Original merged: 2025-11-23T18:02:59Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement, Bug fix ___ ### **Description** - Refactor RPM packaging to support fallback to local execution - Remove hardcoded remote cache disabling step - Add retry logic for remote packaging failures - Update Bazel RPM toolchain flag to use rules_pkg namespace ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Remote packaging attempt"] -->|Success| B["Publish packages"] A -->|Failure| C["Local packaging fallback"] C --> B D["Remove cache disable step"] -.-> A E["Update toolchain flags"] -.-> A ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>release.yml<dd><code>Implement fallback mechanism for RPM packaging</code>                      </dd></summary> <hr> .github/workflows/release.yml <ul><li>Removed the separate step that disabled remote executor/cache settings <li> Refactored packaging logic into two functions: <code>run_remote()</code> and <code>run_local()</code> <li> Updated Bazel RPM toolchain flag from <code>--//toolchains/rpm:is_rpmbuild_available=1</code> to <code>--@rules_pkg//toolchains/rpm:is_rpmbuild_available=1</code> <li> Added conditional retry logic to attempt local packaging if remote execution fails</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1987/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34">+25/-17</a>  </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-23 18:03:02 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1987#issuecomment-3568202247
Original created: 2025-11-23T18:03:02Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Insecure local fallback Description: The workflow retries local Bazel execution on remote failure without isolating or hardening the runner (e.g., no sandbox/SELinux checks, no path restrictions), which could allow unsafe package build scripts to execute with runner privileges and exfiltrate GitHub secrets available to the job. release.yml [257-281] Referred Code `run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } ... (clipped 4 lines)`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Action logging: The new workflow steps add remote/local packaging execution but do not introduce any audit logging of critical actions or outcomes beyond a single generic echo on failure. Referred Code `if ! run_remote; then echo "Remote packaging failed; retrying locally without remote executor/cache" >&2 run_local fi` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Limited retries: The workflow retries locally on remote failure but lacks explicit handling for local failure (e.g., exit status check, contextual error message) and does not surface actionable diagnostics. Referred Code `if ! run_remote; then echo "Remote packaging failed; retrying locally without remote executor/cache" >&2 run_local fi` Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1987#issuecomment-3568202247 Original created: 2025-11-23T18:03:02Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary>Insecure local fallback </summary> Description: The workflow retries local Bazel execution on remote failure without isolating or hardening the runner (e.g., no sandbox/SELinux checks, no path restrictions), which could allow unsafe package build scripts to execute with runner privileges and exfiltrate GitHub secrets available to the job. <a href='https://github.com/carverauto/serviceradar/pull/1987/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R257-R281'>release.yml [257-281]</a> <details open><summary>Referred Code</summary> ```yaml run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } ... (clipped 4 lines) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=2>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1987/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R278-R281'>Action logging</a>: The new workflow steps add remote/local packaging execution but do not introduce any audit logging of critical actions or outcomes beyond a single generic echo on failure. <details open><summary>Referred Code</summary> ```yaml if ! run_remote; then echo "Remote packaging failed; retrying locally without remote executor/cache" >&2 run_local fi ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1987/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R278-R281'>Limited retries</a>: The workflow retries locally on remote failure but lacks explicit handling for local failure (e.g., exit status check, contextual error message) and does not surface actionable diagnostics. <details open><summary>Referred Code</summary> ```yaml if ! run_remote; then echo "Remote packaging failed; retrying locally without remote executor/cache" >&2 run_local fi ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-11-23 18:03:57 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1987#issuecomment-3568203162
Original created: 2025-11-23T18:03:57Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add missing host platform flag Add the missing `--host_platform=@local_config_platform//:host` flag to the `run_remote` function to ensure correct toolchain resolution and prevent potential build failures. .github/workflows/release.yml [257-265] `run_remote() { bazel run \ --config=remote \ + --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" }` `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the `--host_platform` flag was omitted from the new `run_remote` function. This flag was present in the original command and is crucial for correct toolchain resolution, making its absence a likely bug that could cause the remote build to fail.	High
High-level	Refactor duplicated logic into one function The `run_remote` and `run_local` functions contain nearly identical `bazel run` commands. These should be merged into a single parameterized function to reduce redundancy and improve maintainability. Examples: .github/workflows/release.yml [257-276] `run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } ... (clipped 10 lines)` Solution Walkthrough: Before: `run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ ... -- "${args[@]}" } run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ ... -- "${args[@]}" } if ! run_remote; then run_local fi` After: `run_packaging() { local mode=$1 local bazel_args=() if [[ "$mode" == "local" ]]; then bazel_args+=("--config=no_remote" "--host_platform=@local_config_platform//:host") else bazel_args+=("--config=remote") fi bazel run \ "${bazel_args[@]}" \ --platforms=//build/platforms:linux_pkg_local \ ... -- "${args[@]}" } if ! run_packaging "remote"; then echo "Remote packaging failed; retrying locally..." >&2 run_packaging "local" fi` Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies significant code duplication between the `run_remote` and `run_local` functions, and refactoring them would improve the long-term maintainability of the workflow script.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1987#issuecomment-3568203162 Original created: 2025-11-23T18:03:57Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Add missing host platform flag</summary> ___ **Add the missing <code>--host_platform=@local_config_platform//:host</code> flag to the <code>run_remote</code> function to ensure correct toolchain resolution and prevent potential build failures.** [.github/workflows/release.yml [257-265]](https://github.com/carverauto/serviceradar/pull/1987/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R257-R265) ```diff run_remote() { bazel run \ --config=remote \ + --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies that the `--host_platform` flag was omitted from the new `run_remote` function. This flag was present in the original command and is crucial for correct toolchain resolution, making its absence a likely bug that could cause the remote build to fail. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Refactor duplicated logic into one function</summary> ___ **The <code>run_remote</code> and <code>run_local</code> functions contain nearly identical <code>bazel run</code> commands. These should be merged into a single parameterized function to reduce redundancy and improve maintainability.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1987/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R257-R276">.github/workflows/release.yml [257-276]</a> </summary> ```yaml run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --@rules_pkg//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } ... (clipped 10 lines) ``` </details> ### Solution Walkthrough: #### Before: ```yaml run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ ... -- "${args[@]}" } run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ ... -- "${args[@]}" } if ! run_remote; then run_local fi ``` #### After: ```yaml run_packaging() { local mode=$1 local bazel_args=() if [[ "$mode" == "local" ]]; then bazel_args+=("--config=no_remote" "--host_platform=@local_config_platform//:host") else bazel_args+=("--config=remote") fi bazel run \ "${bazel_args[@]}" \ --platforms=//build/platforms:linux_pkg_local \ ... -- "${args[@]}" } if ! run_packaging "remote"; then echo "Remote packaging failed; retrying locally..." >&2 run_packaging "local" fi ``` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies significant code duplication between the `run_remote` and `run_local` functions, and refactoring them would improve the long-term maintainability of the workflow script. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>