carverauto/serviceradar

Fork 0

initial #2265

Merged

mfreeman451 merged 5 commits from refs/pull/2265/head into main

2025-10-04 03:09:25 +00:00

mfreeman451 commented

2025-10-04 01:37:35 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1691
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1691
Original created: 2025-10-04T01:37:35Z
Original updated: 2025-10-04T03:12:07Z
Original head: carverauto/serviceradar:1690-chorebazel-create-github-release
Original base: main
Original merged: 2025-10-04T03:09:25Z by @mfreeman451

PR Type

Enhancement

Description

Add GitHub release publishing automation for ServiceRadar packages
Implement Bazel-based release pipeline with container and artifact publishing
Create comprehensive release documentation and workflow integration
Add version validation and dry-run capabilities

Diagram Walkthrough

flowchart LR
  A["Bazel Build"] --> B["Package Artifacts"]
  B --> C["GitHub Release API"]
  C --> D["Upload Assets"]
  E["Container Images"] --> F["GHCR Registry"]
  G["Release Pipeline"] --> A
  G --> E

File Walkthrough

Relevant files

Enhancement

publish_packages.go `Core GitHub release publishing tool` release/publish_packages.go Implement GitHub API client for release management Add asset upload functionality with overwrite support Create runfile resolver for Bazel artifact discovery Support dry-run mode and comprehensive error handling	+618/-0
release_pipeline.sh `BuildBuddy release automation pipeline` tools/buildbuddy/release_pipeline.sh Create automated release pipeline script Add tag resolution from multiple sources Implement version validation against VERSION file Support configurable release options via environment variables	+115/-0
release_targets.bzl `Bazel packaging targets for releases` packaging/release_targets.bzl Define Bazel rules for package manifest generation Create aggregate targets for Debian and RPM artifacts Implement file listing for release asset discovery	+63/-0

Miscellaneous

VERSION

Version bump to pre-release

VERSION

Update version to pre-release format

+1/-1

Documentation

RELEASE_PUBLISHING.md

Comprehensive release publishing documentation

docs/RELEASE_PUBLISHING.md

Document complete release publishing workflow
Explain Bazel integration and command usage
Cover troubleshooting and BuildBuddy automation
Detail environment variables and configuration options

+91/-0

Configuration changes

BUILD.bazel `Bazel build configuration for publisher` release/BUILD.bazel Define Go binary target for publish_packages Include package artifacts and manifest as data dependencies	+16/-0
BUILD.bazel `BuildBuddy tool build configuration` tools/buildbuddy/BUILD.bazel Add shell binary target for release pipeline	+8/-0

Imported from GitHub pull request. Original GitHub pull request: #1691 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1691 Original created: 2025-10-04T01:37:35Z Original updated: 2025-10-04T03:12:07Z Original head: carverauto/serviceradar:1690-chorebazel-create-github-release Original base: main Original merged: 2025-10-04T03:09:25Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Add GitHub release publishing automation for ServiceRadar packages - Implement Bazel-based release pipeline with container and artifact publishing - Create comprehensive release documentation and workflow integration - Add version validation and dry-run capabilities ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Bazel Build"] --> B["Package Artifacts"] B --> C["GitHub Release API"] C --> D["Upload Assets"] E["Container Images"] --> F["GHCR Registry"] G["Release Pipeline"] --> A G --> E ``` <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>publish_packages.go</strong><dd><code>Core GitHub release publishing tool</code>                                            </dd></summary> <hr> release/publish_packages.go <ul><li>Implement GitHub API client for release management<br> <li> Add asset upload functionality with overwrite support<br> <li> Create runfile resolver for Bazel artifact discovery<br> <li> Support dry-run mode and comprehensive error handling</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7">+618/-0</a>  </td> </tr> <tr> <td> <details> <summary><strong>release_pipeline.sh</strong><dd><code>BuildBuddy release automation pipeline</code>                                      </dd></summary> <hr> tools/buildbuddy/release_pipeline.sh <ul><li>Create automated release pipeline script<br> <li> Add tag resolution from multiple sources<br> <li> Implement version validation against VERSION file<br> <li> Support configurable release options via environment variables</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-6322b9c7846de5d17c06fb5f6facdda53d7503a55d3e1df2b7eb6eed630dc550">+115/-0</a>  </td> </tr> <tr> <td> <details> <summary><strong>release_targets.bzl</strong><dd><code>Bazel packaging targets for releases</code>                                          </dd></summary> <hr> packaging/release_targets.bzl <ul><li>Define Bazel rules for package manifest generation<br> <li> Create aggregate targets for Debian and RPM artifacts<br> <li> Implement file listing for release asset discovery</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-4b654ab00b665505b39b6622594edce8a637859c5d1c075803ef714b2c082d6e">+63/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Miscellaneous</strong></td><td><table> <tr> <td> <details> <summary><strong>VERSION</strong><dd><code>Version bump to pre-release</code>                                                            </dd></summary> <hr> VERSION - Update version to pre-release format </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-7b60b8e351cbb80c47459ffe2c79f1a26404871f49294780fe47ad0e58c09350">+1/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>RELEASE_PUBLISHING.md</strong><dd><code>Comprehensive release publishing documentation</code>                      </dd></summary> <hr> docs/RELEASE_PUBLISHING.md <ul><li>Document complete release publishing workflow<br> <li> Explain Bazel integration and command usage<br> <li> Cover troubleshooting and BuildBuddy automation<br> <li> Detail environment variables and configuration options</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-6ca2ba93be21a4d63c3f8a08ff4889a2979a0c281a4bb70dbe15635bd199bc7e">+91/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>BUILD.bazel</strong><dd><code>Bazel build configuration for publisher</code>                                    </dd></summary> <hr> release/BUILD.bazel <ul><li>Define Go binary target for publish_packages<br> <li> Include package artifacts and manifest as data dependencies</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-fb0e62a1ff706ec8f72e2a1bc655729d1d4fea31e015702ac01e032f67aaf708">+16/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>BUILD.bazel</strong><dd><code>BuildBuddy tool build configuration</code>                                            </dd></summary> <hr> tools/buildbuddy/BUILD.bazel - Add shell binary target for release pipeline </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-f93a8add94ace70ce7023179ed573b3037520bdfcc3563023db76d9232d9a0f1">+8/-0</a>      </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-04 01:38:47 +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/1691#issuecomment-3367744646
Original created: 2025-10-04T01:38:47Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Dry-run bypass risk

Description: GitHub API HTTP client returns a fabricated 200 OK response with empty JSON body during
dry-run, which could mask real server-side validation or permission issues if mistakenly
enabled in production; ensure dry-run cannot be enabled unintentionally in automated
environments.
publish_packages.go [263-295]

Referred Code

func (c *githubClient) request(method, endpoint string, body io.Reader, contentType string) (*http.Response, error) {
	req, err := http.NewRequest(method, endpoint, body)
	if err != nil {
		return nil, err
	}

	if !c.dryRun {
		if c.token != "" {
			req.Header.Set("Authorization", "Bearer "+c.token)
		}
	}
	req.Header.Set("Accept", "application/vnd.github+json")
	req.Header.Set("X-GitHub-Api-Version", "2022-11-28")
	if contentType != "" {
		req.Header.Set("Content-Type", contentType)
	}

	if c.dryRun {
		fmt.Printf("[dry-run] %s %s\n", method, endpoint)
		return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("{}"))}, nil
	}


 ... (clipped 12 lines)

Missing integrity checks

Description: Asset upload omits checksum verification or content hashing, making it harder to detect
tampering or corruption of artifacts during transit or from disk; consider computing and
uploading checksums or verifying sizes post-upload.
publish_packages.go [383-439]

Referred Code

func (c *githubClient) uploadAsset(uploadURL, assetPath string) error {
	if uploadURL == "" {
		return errors.New("upload URL is empty")
	}

	base := uploadURL
	if idx := strings.Index(uploadURL, "{"); idx != -1 {
		base = uploadURL[:idx]
	}

	name := filepath.Base(assetPath)
	endpoint := fmt.Sprintf("%s?name=%s", strings.TrimRight(base, "/"), url.QueryEscape(name))

	file, err := os.Open(assetPath)
	if err != nil {
		return err
	}
	defer file.Close()

	info, err := file.Stat()
	if err != nil {


 ... (clipped 36 lines)

Unsafe tag fallback

Description: Tag derivation falls back to environment variables and defaults (e.g., sha-dev) which, if
misconfigured, could cause publishing under unintended tags; add stricter validation in
non-dry-run mode to prevent accidental releases.
release_pipeline.sh [21-44]

Referred Code

# Determine release tag, preferring explicit overrides.
TAG_CANDIDATES=(
    "${RELEASE_TAG:-}"
    "${WORKFLOW_INPUT_tag:-}"
    "${WORKFLOW_TAG:-}"
    "${GIT_TAG:-}"
    "${GITHUB_REF_NAME:-}"
)
TAG=""
for candidate in "${TAG_CANDIDATES[@]}"; do
    if [[ -n "${candidate}" ]]; then
        TAG="${candidate}"
        break
    fi
done
if [[ -z "${TAG}" ]]; then
    if [[ -f "${REPO_ROOT}/VERSION" ]]; then
        TAG="$(<"${REPO_ROOT}/VERSION")"
    elif [[ -n "${GIT_COMMIT:-}" ]]; then
        TAG="sha-${GIT_COMMIT}"
    else


 ... (clipped 3 lines)

Ticket Compliance

🟡

🎫 #1690

🟢	Create GitHub release publishing automation for ServiceRadar packages.
	Implement a Bazel-based release pipeline that builds and publishes container images and package artifacts.
	Provide documentation for the release workflow and integration.
	Include version validation and support for dry-run mode.
⚪	Validate end-to-end in CI/workflow environment that release creation, asset uploads, and container pushes work with real credentials.
⚪	Confirm permissions and tokens (GHCR and GitHub) are correctly configured in the deployment environment.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

⚪

No custom compliance provided

Follow the guide to enable custom compliance check.

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/1691#issuecomment-3367744646 Original created: 2025-10-04T01:38:47Z --- ## 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 rowspan=3>⚪</td> <td><details><summary><strong>Dry-run bypass risk </strong></summary><br> <b>Description:</b> GitHub API HTTP client returns a fabricated 200 OK response with empty JSON body during <br>dry-run, which could mask real server-side validation or permission issues if mistakenly <br>enabled in production; ensure dry-run cannot be enabled unintentionally in automated <br>environments.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7R263-R295'>publish_packages.go [263-295]</a></strong><br> <details open><summary>Referred Code</summary> ```go func (c *githubClient) request(method, endpoint string, body io.Reader, contentType string) (*http.Response, error) { req, err := http.NewRequest(method, endpoint, body) if err != nil { return nil, err } if !c.dryRun { if c.token != "" { req.Header.Set("Authorization", "Bearer "+c.token) } } req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("X-GitHub-Api-Version", "2022-11-28") if contentType != "" { req.Header.Set("Content-Type", contentType) } if c.dryRun { fmt.Printf("[dry-run] %s %s\n", method, endpoint) return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("{}"))}, nil } ... (clipped 12 lines) ``` </details></details></td></tr> <tr><td><details><summary><strong>Missing integrity checks </strong></summary><br> <b>Description:</b> Asset upload omits checksum verification or content hashing, making it harder to detect <br>tampering or corruption of artifacts during transit or from disk; consider computing and <br>uploading checksums or verifying sizes post-upload.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7R383-R439'>publish_packages.go [383-439]</a></strong><br> <details open><summary>Referred Code</summary> ```go func (c *githubClient) uploadAsset(uploadURL, assetPath string) error { if uploadURL == "" { return errors.New("upload URL is empty") } base := uploadURL if idx := strings.Index(uploadURL, "{"); idx != -1 { base = uploadURL[:idx] } name := filepath.Base(assetPath) endpoint := fmt.Sprintf("%s?name=%s", strings.TrimRight(base, "/"), url.QueryEscape(name)) file, err := os.Open(assetPath) if err != nil { return err } defer file.Close() info, err := file.Stat() if err != nil { ... (clipped 36 lines) ``` </details></details></td></tr> <tr><td><details><summary><strong>Unsafe tag fallback </strong></summary><br> <b>Description:</b> Tag derivation falls back to environment variables and defaults (e.g., sha-dev) which, if <br>misconfigured, could cause publishing under unintended tags; add stricter validation in <br>non-dry-run mode to prevent accidental releases.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1691/files#diff-6322b9c7846de5d17c06fb5f6facdda53d7503a55d3e1df2b7eb6eed630dc550R21-R44'>release_pipeline.sh [21-44]</a></strong><br> <details open><summary>Referred Code</summary> ```shell # Determine release tag, preferring explicit overrides. TAG_CANDIDATES=( "${RELEASE_TAG:-}" "${WORKFLOW_INPUT_tag:-}" "${WORKFLOW_TAG:-}" "${GIT_TAG:-}" "${GITHUB_REF_NAME:-}" ) TAG="" for candidate in "${TAG_CANDIDATES[@]}"; do if [[ -n "${candidate}" ]]; then TAG="${candidate}" break fi done if [[ -z "${TAG}" ]]; then if [[ -f "${REPO_ROOT}/VERSION" ]]; then TAG="$(<"${REPO_ROOT}/VERSION")" elif [[ -n "${GIT_COMMIT:-}" ]]; then TAG="sha-${GIT_COMMIT}" else ... (clipped 3 lines) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/1690>#1690</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Create GitHub release publishing automation for ServiceRadar packages.</td></tr> <tr><td>Implement a Bazel-based release pipeline that builds and publishes container images and <br>package artifacts.</td></tr> <tr><td>Provide documentation for the release workflow and integration.</td></tr> <tr><td>Include version validation and support for dry-run mode.</td></tr> <tr><td rowspan=2>⚪</td> <td>Validate end-to-end in CI/workflow environment that release creation, asset uploads, and <br>container pushes work with real credentials.</td></tr> <tr><td>Confirm permissions and tokens (GHCR and GitHub) are correctly configured in the <br>deployment environment.</td></tr> </tbody></table> </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>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </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-10-04 01:39:55 +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/1691#issuecomment-3367745277
Original created: 2025-10-04T01:39:55Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Consider using the official GitHub CLI Replace the custom 600-line Go program for GitHub API interaction with the official GitHub CLI (`gh`). This would reduce maintenance by leveraging a standard, robust tool for release creation and asset uploads. Examples: release/publish_packages.go [1-618] `package main import ( "bufio" "bytes" "encoding/json" "errors" "flag" "fmt" "io" ... (clipped 608 lines)` Solution Walkthrough: Before: // release/publish_packages.go func main() { // ... parse ~10 command-line flags ... // ... resolve bazel runfiles to find artifacts ... assets, err := collectAssets(...) // ... custom GitHub client initialization ... client := newGithubClient(token, repo, *dryRunFlag) // ... ensure release exists (create or update) ... rel, _, err := ensureRelease(client, ...) // ... loop through artifacts and upload them ... for _, artifact := range assets { // ... check if asset exists, delete if overwriting ... if err := client.uploadAsset(rel.UploadURL, artifact); err != nil { // ... handle error ... } } } // ... ~500 more lines of custom GitHub API client logic ... After: # In a shell script like release_pipeline.sh # ... determine TAG, NOTES_FILE, etc. from env vars ... # Create or update the release using GitHub CLI gh release create "$TAG" \ --repo "$REPO" \ --title "$NAME" \ --notes-file "$NOTES_FILE" \ --draft \ --prerelease # Find artifacts from bazel-generated manifest ARTIFACT_MANIFEST="bazel-bin/release/package_manifest.txt" bazel build //release:package_manifest # Upload each artifact using GitHub CLI gh release upload "$TAG" $(cat "$ARTIFACT_MANIFEST") --clobber Suggestion importance[1-10]: 9 __ Why: This is an excellent architectural suggestion that correctly identifies the custom Go program (`publish_packages.go`) reinvents functionality provided by the standard GitHub CLI (`gh`), and replacing it would significantly reduce code complexity and long-term maintenance.	High
General	✅ ~~Handle potential error when reading response~~ Suggestion Impact: The commit modified the error handling path after non-2xx responses: it still reads the body without checking the read error, but it changed the returned error to wrap a sentinel error (errGithubAPI). Although it did not implement the exact suggested check for io.ReadAll's error, it altered the same code block and adjusted error reporting, indicating the suggestion influenced the change. code diff: `@@ -287,9 +287,11 @@ return nil, err } if resp.StatusCode >= 400 && resp.StatusCode != http.StatusNotFound { - defer resp.Body.Close() + defer func() { + _ = resp.Body.Close() + }() data, _ := io.ReadAll(resp.Body) - return nil, fmt.Errorf("github api error: %s (%s)", resp.Status, strings.TrimSpace(string(data))) + return nil, fmt.Errorf("%w: %s (%s)", errGithubAPI, resp.Status, strings.TrimSpace(string(data))) }` Handle the potential error returned by `io.ReadAll` when reading an API error response body to ensure error messages are complete and accurate. release/publish_packages.go [289-293] `if resp.StatusCode >= 400 && resp.StatusCode != http.StatusNotFound { defer resp.Body.Close() - data, _ := io.ReadAll(resp.Body) + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("github api error: %s (failed to read error body: %v)", resp.Status, err) + } return nil, fmt.Errorf("github api error: %s (%s)", resp.Status, strings.TrimSpace(string(data))) }` `[Suggestion processed]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that an error from `io.ReadAll` is ignored, which could result in losing important diagnostic information. Handling this error makes the error reporting more robust.	Medium
	Remove redundant error handling logic Remove the redundant `errors.As` check in the error handling block, as it returns the original error regardless of the outcome, simplifying the code. release/publish_packages.go [300-306] `if err != nil { - var apiErr url.Error - if errors.As(err, &apiErr) { - return nil, err - } return nil, err }` Apply / Chat* Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a redundant block of error-checking code that has no effect on the program's logic. Removing it improves code clarity and simplicity.	Low
	Remove duplicate code block Remove the duplicate code block that checks and resets the `PUSH_ARGS` array, as this action is already performed earlier in the script. tools/buildbuddy/release_pipeline.sh [104-106] `-if [[ ${#PUSH_ARGS[@]} -eq 1 && -z "${PUSH_ARGS[0]}" ]]; then - PUSH_ARGS=() -fi +# This block is a duplicate and can be removed.` Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a duplicated block of code that resets the `PUSH_ARGS` array. Removing the redundant block simplifies the script and improves maintainability.	Low
Possible issue	Use case-sensitive commit SHA comparison Replace the case-insensitive commit SHA comparison using `strings.EqualFold` with a standard case-sensitive comparison (`!=`) for improved correctness. release/publish_packages.go [224-226] `- if args.commit != "" && !strings.EqualFold(args.commit, existing.TargetCommitish) { + if args.commit != "" && args.commit != existing.TargetCommitish { needUpdate = true }` Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that using a case-insensitive comparison for commit SHAs is not best practice. Switching to a case-sensitive comparison with `!=` improves the code's robustness and correctness.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1691#issuecomment-3367745277 Original created: 2025-10-04T01:39:55Z --- ## 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>High-level</td> <td> <details><summary>Consider using the official GitHub CLI</summary> ___ **Replace the custom 600-line Go program for GitHub API interaction with the <br>official GitHub CLI (<code>gh</code>). This would reduce maintenance by leveraging a <br>standard, robust tool for release creation and asset uploads.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7R1-R618">release/publish_packages.go [1-618]</a> </summary> ```go package main import ( "bufio" "bytes" "encoding/json" "errors" "flag" "fmt" "io" ... (clipped 608 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // release/publish_packages.go func main() { // ... parse ~10 command-line flags ... // ... resolve bazel runfiles to find artifacts ... assets, err := collectAssets(...) // ... custom GitHub client initialization ... client := newGithubClient(token, repo, *dryRunFlag) // ... ensure release exists (create or update) ... rel, _, err := ensureRelease(client, ...) // ... loop through artifacts and upload them ... for _, artifact := range assets { // ... check if asset exists, delete if overwriting ... if err := client.uploadAsset(rel.UploadURL, artifact); err != nil { // ... handle error ... } } } // ... ~500 more lines of custom GitHub API client logic ... ``` #### After: ```go # In a shell script like release_pipeline.sh # ... determine TAG, NOTES_FILE, etc. from env vars ... # Create or update the release using GitHub CLI gh release create "$TAG" \ --repo "$REPO" \ --title "$NAME" \ --notes-file "$NOTES_FILE" \ --draft \ --prerelease # Find artifacts from bazel-generated manifest ARTIFACT_MANIFEST="bazel-bin/release/package_manifest.txt" bazel build //release:package_manifest # Upload each artifact using GitHub CLI gh release upload "$TAG" $(cat "$ARTIFACT_MANIFEST") --clobber ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This is an excellent architectural suggestion that correctly identifies the custom Go program (`publish_packages.go`) reinvents functionality provided by the standard GitHub CLI (`gh`), and replacing it would significantly reduce code complexity and long-term maintenance. </details></details></td><td align=center>High </td></tr><tr><td rowspan=3>General</td> <td> <details><summary>✅ <s>Handle potential error when reading response</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit modified the error handling path after non-2xx responses: it still reads the body without checking the read error, but it changed the returned error to wrap a sentinel error (errGithubAPI). Although it did not implement the exact suggested check for io.ReadAll's error, it altered the same code block and adjusted error reporting, indicating the suggestion influenced the change. code diff: ```diff @@ -287,9 +287,11 @@ return nil, err } if resp.StatusCode >= 400 && resp.StatusCode != http.StatusNotFound { - defer resp.Body.Close() + defer func() { + _ = resp.Body.Close() + }() data, _ := io.ReadAll(resp.Body) - return nil, fmt.Errorf("github api error: %s (%s)", resp.Status, strings.TrimSpace(string(data))) + return nil, fmt.Errorf("%w: %s (%s)", errGithubAPI, resp.Status, strings.TrimSpace(string(data))) } ``` </details> ___ **Handle the potential error returned by <code>io.ReadAll</code> when reading an API error <br>response body to ensure error messages are complete and accurate.** [release/publish_packages.go [289-293]](https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7R289-R293) ```diff if resp.StatusCode >= 400 && resp.StatusCode != http.StatusNotFound { defer resp.Body.Close() - data, _ := io.ReadAll(resp.Body) + data, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("github api error: %s (failed to read error body: %v)", resp.Status, err) + } return nil, fmt.Errorf("github api error: %s (%s)", resp.Status, strings.TrimSpace(string(data))) } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that an error from `io.ReadAll` is ignored, which could result in losing important diagnostic information. Handling this error makes the error reporting more robust. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Remove redundant error handling logic</summary> ___ **Remove the redundant <code>errors.As</code> check in the error handling block, as it returns <br>the original error regardless of the outcome, simplifying the code.** [release/publish_packages.go [300-306]](https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7R300-R306) ```diff if err != nil { - var apiErr *url.Error - if errors.As(err, &apiErr) { - return nil, err - } return nil, err } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly identifies a redundant block of error-checking code that has no effect on the program's logic. Removing it improves code clarity and simplicity. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Remove duplicate code block</summary> ___ **Remove the duplicate code block that checks and resets the <code>PUSH_ARGS</code> array, as <br>this action is already performed earlier in the script.** [tools/buildbuddy/release_pipeline.sh [104-106]](https://github.com/carverauto/serviceradar/pull/1691/files#diff-6322b9c7846de5d17c06fb5f6facdda53d7503a55d3e1df2b7eb6eed630dc550R104-R106) ```diff -if [[ ${#PUSH_ARGS[@]} -eq 1 && -z "${PUSH_ARGS[0]}" ]]; then - PUSH_ARGS=() -fi +# This block is a duplicate and can be removed. ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly identifies a duplicated block of code that resets the `PUSH_ARGS` array. Removing the redundant block simplifies the script and improves maintainability. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Use case-sensitive commit SHA comparison</summary> ___ **Replace the case-insensitive commit SHA comparison using <code>strings.EqualFold</code> with <br>a standard case-sensitive comparison (<code>!=</code>) for improved correctness.** [release/publish_packages.go [224-226]](https://github.com/carverauto/serviceradar/pull/1691/files#diff-638cfa7da020be995c5e05ba794edd1080d194845870a0061688e0b9afbcadc7R224-R226) ```diff - if args.commit != "" && !strings.EqualFold(args.commit, existing.TargetCommitish) { + if args.commit != "" && args.commit != existing.TargetCommitish { needUpdate = true } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that using a case-insensitive comparison for commit SHAs is not best practice. Switching to a case-sensitive comparison with `!=` improves the code's robustness and correctness. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>