Use bazel cquery to find out the binary path instead of guessing

[apesternikov]

We were using heuristics to guess a binary for a target.
It is possible to use cquery to get the exact path instead of guessing.
As a side effect of this change it does not depend on protobuf anymore so we did a cleanup

[codereviewbot-ai]

In ExWithTimeout, errors resulting from context timeout or cancellation are not distinguished from other errors. This can make it difficult to diagnose whether a failure was due to a timeout or another issue. Consider checking if the error is context.DeadlineExceeded or context.Canceled and returning a more descriptive error message.

Recommended solution:

if ctx.Err() != nil {
    return string(b), fmt.Errorf("command timed out or was canceled: %w", ctx.Err())
}

[codereviewbot-ai]

The use of log.Fatalf in Mustex and MustexWithTimeout will terminate the process on error, which may not be appropriate for library code and reduces flexibility for callers. Additionally, the error message does not include the command or arguments that failed, making debugging more difficult. Consider returning the error to the caller or including more context in the fatal log message.

Recommended solution:

log.Fatalf("ERROR executing %s %v: %s", name, arg, err)

Or, if possible, return the error instead of exiting.

[codereviewbot-ai]

Potential Data Race on Global Timeout Variable

The test manipulates the global Timeout variable directly without synchronization:

origTimeout := *Timeout
defer func() { *Timeout = origTimeout }()
*Timeout = 1 * time.Microsecond

If tests are run in parallel, this can lead to data races and inconsistent results. To mitigate this, consider isolating the timeout per test or using synchronization primitives (e.g., mutex) to protect access to the global variable. Alternatively, refactor the code to avoid global state in tests.

[codereviewbot-ai]

Issue: Lack of Error Handling in Parallel Push Logic

In the parallel execution block for pushing binaries, errors from goroutines are not collected or handled. The use of exec.Mustex may terminate the process on error, but if it does not, failures are silently ignored, leading to undetected push failures.

Recommendation:

• Use a channel or an error group (e.g., errgroup.Group) to collect errors from each goroutine.
• After all goroutines complete, check for errors and handle them appropriately, possibly aborting or retrying as needed.

Example:

var eg errgroup.Group
for _, path := range paths {
    p := path
    eg.Go(func() error {
        // ...
        if err := exec.MayFail(...); err != nil {
            return err
        }
        return nil
    })
}
if err := eg.Wait(); err != nil {
    log.Fatalf("Push failed: %v", err)
}

This ensures all errors are surfaced and handled.

[codereviewbot-ai]

Issue: Abrupt Process Termination on Errors

Throughout the code (e.g., in bazelQueryTargets, bazelQueryPaths, and the main release train logic), errors are handled using log.Fatal or exec.Mustex, which abruptly terminate the process. This approach prevents graceful error handling, makes it difficult to report partial results, and complicates debugging in batch or CI environments.

Recommendation:

• Refactor functions to return errors instead of calling log.Fatal or using Mustex.
• In the main logic, aggregate and report errors, allowing the process to continue where possible or to provide a comprehensive error report at the end.

Example:

func bazelQueryTargets(query string) ([]GitopsTarget, error) {
    // ...
    if err != nil {
        return nil, err
    }
    // ...
}

This enables more robust and maintainable error handling.

[codereviewbot-ai]

Fragile Regex Pattern for Push Binary Verification

The regex pattern constructed for verifying push binaries (lines 65-67) only escapes periods (.) in the workspace and binary paths. If either path contains other regex special characters (such as +, *, ?, (, ), [, ], {, }, |, ^, $), the pattern may not match as intended, leading to false negatives or positives.

Recommendation:
Use a more robust escaping mechanism for all regex special characters. For example, use sed -e 's/[]\^$.|?*+(){}[]/\\&/g' to escape all regex metacharacters:

escaped_workspace=$(echo "$WORKSPACE_ROOT" | sed -e 's/[]\\^$.|?*+(){}[]/\\&/g')
escaped_bin=$(echo "$push_bin" | sed -e 's/[]\\^$.|?*+(){}[]/\\&/g')

This will ensure the pattern matches the intended literal paths.

[codereviewbot-ai]

Early Exit on First Failure Reduces Test Utility

Both verification loops (lines 56-61 and 64-71) exit immediately upon the first missing target or binary. This prevents the script from reporting all missing items in a single run, making debugging less efficient.

Recommendation:
Accumulate errors in a variable and report all missing targets and binaries at the end of the script, exiting with a non-zero status if any were missing. For example:

errors=0
# ... inside each loop ...
  errors=1
# ... after loops ...
if [ "$errors" -ne 0 ]; then
  exit 1
fi

This approach provides a comprehensive error report for faster troubleshooting.

[codereviewbot-ai]

Potential Issue: Incorrect Parsing of Resolved Binaries

The parsing logic for resolvedBinaries uses strings.Cut(rb, ":"), which only splits on the first colon. If the binary path contains additional colons (e.g., in Windows paths or container image references), this may lead to incorrect parsing and assignment.

Recommendation:

• Use a more robust parsing method, such as splitting only on the first colon and treating the remainder as the binary path, or validating the input format more strictly.

Example:

releaseTrain, bin, found := strings.Cut(rb, ":")
if !found || releaseTrain == "" || bin == "" {
    log.Fatalf("resolved_binaries: invalid resolved_binary format: %s", rb)
}

Or use regex for stricter validation.

[codereviewbot-ai]

Logic Issue: Branch Recreation May Not Handle All Deleted Targets

The branch recreation logic only recreates the branch if the first missing target is found, then breaks. If multiple targets are deleted, only the first is considered, and the branch is recreated once. This may not fully synchronize the branch state with the current targets, potentially leaving stale commits or metadata.

Recommendation:

• After recreating the branch, ensure that all deleted targets are handled and the branch state is fully synchronized. Consider re-checking the target set after recreation or performing a full reset.

Example:

for _, t := range oldtargets {
    if !targetset[t] {
        workdir.RecreateBranch(branch, *prInto)
        // Optionally, re-check or reset branch state here
        break
    }
}

Ensure that the branch reflects the current set of targets after recreation.