carverauto/serviceradar

Fork 0

Fix/rbe rpm #2456

Merged

mfreeman451 merged 3 commits from refs/pull/2456/head into main

2025-11-23 21:47:55 +00:00

mfreeman451 commented

2025-11-23 21:47:07 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1988
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1988
Original created: 2025-11-23T21:47:07Z
Original updated: 2025-11-23T21:48:13Z
Original head: carverauto/serviceradar:fix/rbe_rpm
Original base: main
Original merged: 2025-11-23T21:47:55Z 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

Implement fallback mechanism for RPM packaging with remote retry logic
Remove hardcoded remote executor/cache disabling step
Add conditional execution: attempt remote build first, fallback to local
Improve error handling and logging for packaging failures

Diagram Walkthrough

flowchart LR
  A["Packaging Job"] --> B["Attempt Remote Build"]
  B -->|Success| C["Complete"]
  B -->|Failure| D["Fallback to Local Build"]
  D --> C

File Walkthrough

Relevant files

Enhancement

release.yml

Implement remote-first with local fallback for packaging

.github/workflows/release.yml

Removed static .bazelrc.remote configuration step that disabled remote
settings
Refactored packaging command into two functions: run_remote() and
run_local()
Added conditional logic to attempt remote packaging first, then
fallback to local execution on failure
Improved error messaging to indicate when remote packaging fails and
local retry is triggered
Removed --host_platform flag from remote build configuration

+23/-17

Imported from GitHub pull request. Original GitHub pull request: #1988 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1988 Original created: 2025-11-23T21:47:07Z Original updated: 2025-11-23T21:48:13Z Original head: carverauto/serviceradar:fix/rbe_rpm Original base: main Original merged: 2025-11-23T21:47:55Z 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** - Implement fallback mechanism for RPM packaging with remote retry logic - Remove hardcoded remote executor/cache disabling step - Add conditional execution: attempt remote build first, fallback to local - Improve error handling and logging for packaging failures ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Packaging Job"] --> B["Attempt Remote Build"] B -->|Success| C["Complete"] B -->|Failure| D["Fallback to Local Build"] D --> C ``` <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><table> <tr> <td> <details> <summary><strong>release.yml</strong><dd><code>Implement remote-first with local fallback for packaging</code>  </dd></summary> <hr> .github/workflows/release.yml <ul><li>Removed static <code>.bazelrc.remote</code> configuration step that disabled remote <br>settings<br> <li> Refactored packaging command into two functions: <code>run_remote()</code> and <br><code>run_local()</code><br> <li> Added conditional logic to attempt remote packaging first, then <br>fallback to local execution on failure<br> <li> Improved error messaging to indicate when remote packaging fails and <br>local retry is triggered<br> <li> Removed <code>--host_platform</code> flag from remote build configuration</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1988/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34">+23/-17</a>  </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-23 21:47:27 +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/1988#issuecomment-3568367125
Original created: 2025-11-23T21:47:27Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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: Missing audit logs: The new fallback logic for publishing packages runs remote then local without adding structured audit logs identifying actor, timestamp, action, and outcome for these critical release actions. Referred Code `run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --stamp \ //release:publish_packages \ -- "${args[@]}" } run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ --stamp \ //release:publish_packages \ -- "${args[@]}" } if ! run_remote; then echo "Remote packaging failed; retrying locally without remote executor/cache" >&2 ... (clipped 2 lines)` 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 failure context: The fallback echoes a generic message and retries locally but does not capture or log the remote error output/exit code, which may hinder debugging and monitoring of packaging failures. 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
Update

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/1988#issuecomment-3568367125 Original created: 2025-11-23T21:47:27Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></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'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > 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><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > 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><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > 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><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > 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><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1988/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R257-R279'><strong>Missing audit logs</strong></a>: The new fallback logic for publishing packages runs remote then local without adding <br>structured audit logs identifying actor, timestamp, action, and outcome for these critical <br>release actions.<br> <details open><summary>Referred Code</summary> ```yaml run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --stamp \ //release:publish_packages \ -- "${args[@]}" } run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ --stamp \ //release:publish_packages \ -- "${args[@]}" } if ! run_remote; then echo "Remote packaging failed; retrying locally without remote executor/cache" >&2 ... (clipped 2 lines) ``` </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><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/1988/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R276-R279'><strong>Limited failure context</strong></a>: The fallback echoes a generic message and retries locally but does not capture or log the <br>remote error output/exit code, which may hinder debugging and monitoring of packaging <br>failures.<br> <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"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-11-23 21:48:13 +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/1988#issuecomment-3568367653
Original created: 2025-11-23T21:48:13Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Restore flag for local RPM builds Re-add the `--//toolchains/rpm:is_rpmbuild_available=1` flag to the `run_local` function to ensure the local fallback for RPM packaging works correctly. .github/workflows/release.yml [266-274] `run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ + --//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" }` Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that a crucial build flag (`--//toolchains/rpm:is_rpmbuild_available=1`) was omitted from the new `run_local` function, which could cause the fallback packaging process to fail or silently skip creating RPMs.	High
High-level	Use native GitHub Actions features Refactor the shell-based fallback logic to use native GitHub Actions features. This involves using two separate steps, with the first having `continue-on-error:` `true` and the second executing conditionally via `if: failure()`. Examples: .github/workflows/release.yml [257-279] `run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --stamp \ //release:publish_packages \ -- "${args[@]}" } run_local() { ... (clipped 13 lines)` Solution Walkthrough: Before: `- name: Publish Debian and RPM packages run: \| run_remote() { bazel run --config=remote ... } run_local() { bazel run --config=no_remote ... } if ! run_remote; then echo "Remote packaging failed; retrying locally..." run_local fi` After: `- name: Attempt remote build id: remote_build continue-on-error: true run: \| bazel run --config=remote ... - name: Fallback to local build if: steps.remote_build.outcome == 'failure' run: \| echo "Remote packaging failed; retrying locally..." bazel run --config=no_remote ...` Suggestion importance[1-10]: 8 __ Why: This is a strong architectural suggestion that improves the workflow's clarity, maintainability, and observability by replacing shell-level logic with a more idiomatic and transparent native GitHub Actions pattern.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1988#issuecomment-3568367653 Original created: 2025-11-23T21:48:13Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Restore flag for local RPM builds</summary> ___ **Re-add the <code>--//toolchains/rpm:is_rpmbuild_available=1</code> flag to the <code>run_local</code> <br>function to ensure the local fallback for RPM packaging works correctly.** [.github/workflows/release.yml [266-274]](https://github.com/carverauto/serviceradar/pull/1988/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R266-R274) ```diff run_local() { bazel run \ --config=no_remote \ --host_platform=@local_config_platform//:host \ --platforms=//build/platforms:linux_pkg_local \ + --//toolchains/rpm:is_rpmbuild_available=1 \ --stamp \ //release:publish_packages \ -- "${args[@]}" } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies that a crucial build flag (`--//toolchains/rpm:is_rpmbuild_available=1`) was omitted from the new `run_local` function, which could cause the fallback packaging process to fail or silently skip creating RPMs. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Use native GitHub Actions features</summary> ___ **Refactor the shell-based fallback logic to use native GitHub Actions features. <br>This involves using two separate steps, with the first having <code>continue-on-error: </code><br><code>true</code> and the second executing conditionally via <code>if: failure()</code>.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1988/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R257-R279">.github/workflows/release.yml [257-279]</a> </summary> ```yaml run_remote() { bazel run \ --config=remote \ --platforms=//build/platforms:linux_pkg_local \ --stamp \ //release:publish_packages \ -- "${args[@]}" } run_local() { ... (clipped 13 lines) ``` </details> ### Solution Walkthrough: #### Before: ```yaml - name: Publish Debian and RPM packages run: | run_remote() { bazel run --config=remote ... } run_local() { bazel run --config=no_remote ... } if ! run_remote; then echo "Remote packaging failed; retrying locally..." run_local fi ``` #### After: ```yaml - name: Attempt remote build id: remote_build continue-on-error: true run: | bazel run --config=remote ... - name: Fallback to local build if: steps.remote_build.outcome == 'failure' run: | echo "Remote packaging failed; retrying locally..." bazel run --config=no_remote ... ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This is a strong architectural suggestion that improves the workflow's clarity, maintainability, and observability by replacing shell-level logic with a more idiomatic and transparent native GitHub Actions pattern. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>