run prer from a rule #26
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| if err := os.RemoveAll(dir); err != nil { | ||
| return nil, fmt.Errorf("Unable to clone repo: %w", err) | ||
| return nil, fmt.Errorf("unable to clone repo: %w", err) |
There was a problem hiding this comment.
The use of os.RemoveAll(dir) to clean the directory before cloning the repository poses a significant risk of data loss if the dir variable contains an incorrect path, especially if it points to a critical directory. A safer approach would be to verify the contents of the directory or ensure it is within a controlled environment before deletion. Consider implementing checks or confirmations before performing such destructive operations.
| if err := ioutil.WriteFile(filepath.Join(dir, ".git/info/sparse-checkout"), []byte(genPath), 0644); err != nil { | ||
| return nil, fmt.Errorf("Unable to create .git/info/sparse-checkout: %w", err) | ||
| if err := os.WriteFile(filepath.Join(dir, ".git/info/sparse-checkout"), []byte(genPath), 0644); err != nil { | ||
| return nil, fmt.Errorf("unable to create .git/info/sparse-checkout: %w", err) | ||
| } | ||
| exec.Mustex(dir, "git", "checkout", primaryBranch) |
There was a problem hiding this comment.
The function exec.Mustex is used without error handling, which could lead to the program continuing execution even if a critical error occurs during the git operations (e.g., clone, config, checkout). This could result in a partially cloned or misconfigured repository without the user being aware of the failure. It is recommended to handle errors returned by these operations to ensure the integrity of the cloning process and provide clear feedback to the user in case of failure.
| for _, rp := range resolvedPushes { | ||
| cmd := rp | ||
| eg.Go(func() error { | ||
| exec.Mustex("", cmd) |
There was a problem hiding this comment.
The use of exec.Mustex without handling its potential errors or return values can lead to uncaught exceptions and the program exiting unexpectedly. It's crucial to handle errors gracefully in a production environment to ensure reliability and robustness. Consider capturing the error returned by exec.Mustex and handling it appropriately, such as logging the error and continuing with the next iteration or implementing a retry mechanism.
| bin := bazel.TargetToExecutable(target) | ||
| fi, err := os.Stat(bin) | ||
| if err == nil && fi.Mode().IsRegular() { | ||
| exec.Mustex("", bin) |
There was a problem hiding this comment.
Similar to the previous issue, the use of exec.Mustex here also does not handle potential errors or return values. This can cause the program to exit unexpectedly if an error occurs during the execution of the binary or command. It's important to handle these errors properly to maintain the stability of the application. Consider capturing and handling the error returned by exec.Mustex, possibly by logging the error and deciding on the next steps based on the nature of the error (e.g., retrying the operation, skipping to the next item, or failing gracefully).
| if err != nil { | ||
| log.Fatalf("ERROR: %s", err) |
There was a problem hiding this comment.
The use of log.Fatalf inside the Mustex function to handle errors is problematic for a couple of reasons. Firstly, it will cause the entire program to exit if the command execution fails, which might not be the desired behavior in all contexts where Mustex is used. This approach reduces the flexibility of error handling, making it impossible for calling code to recover from the error or take alternative actions. Secondly, abruptly terminating the program can make it harder to perform cleanup operations or gracefully shutdown other parts of the application.
Recommended Solution: Instead of terminating the program, consider returning the error to the caller. This allows the caller to decide how to handle the error, whether it be logging the error, retrying the operation, or gracefully shutting down. If the intention is to have a version of the function that does not return an error to the caller, you could provide two versions of the function: one that returns an error and one that does not, clearly documenting the behavior of each.
| if _, err = os.Stat(dir + "/.git"); os.IsNotExist(err) { | ||
| newRepo = true | ||
| if err = os.MkdirAll(filepath.Dir(dir), os.ModePerm); err != nil && !os.IsExist(err) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The error handling for os.MkdirAll checks for !os.IsExist(err) which might not be sufficient to catch all relevant errors. Specifically, this condition only checks if the error is not about the directory already existing, but it does not ensure that the directory was successfully created or that it is writable. This could lead to a scenario where the directory creation fails for a reason other than it already existing, but the error is not properly handled, potentially leading to more cryptic errors downstream when trying to use the directory.
A more robust approach would be to handle any error returned by os.MkdirAll directly, without trying to filter it based on its type. If the directory's existence is crucial for the operation to proceed, additional checks should be performed to ensure not only its existence but also its accessibility and suitability for the intended operations.
| if gitopsdir == "" { | ||
| var err error | ||
| gitopsdir, err = os.MkdirTemp(*gitopsTmpDir, "gitops") | ||
| if err != nil { | ||
| log.Fatalf("Unable to create tempdir in %s: %v", *gitopsTmpDir, err) | ||
| } | ||
| defer os.RemoveAll(gitopsdir) |
There was a problem hiding this comment.
The use of defer os.RemoveAll(gitopsdir) immediately after creating a temporary directory for git operations introduces a risk of deleting important files inadvertently if the gitopsdir variable gets manipulated elsewhere in the code. This could lead to data loss, especially if the directory contains important git operations or configurations. To mitigate this risk, ensure that the temporary directory is only removed after all necessary operations have been completed and verified. Additionally, consider implementing more robust error handling and validation around the usage of temporary directories to prevent unintended side effects.
| workdir, err := git.CloneOrCheckout(*repo, gitopsdir, *gitMirror, *prInto, *gitopsPath, *deployBranchPrefix) | ||
| if err != nil { | ||
| log.Fatalf("Unable to clone repo: %v", err) |
There was a problem hiding this comment.
The use of log.Fatalf for error handling within the git.CloneOrCheckout function call can cause the program to exit immediately without performing any cleanup or finalization operations that might be necessary. This abrupt termination could leave behind resources or temporary files that were intended to be cleaned up, leading to potential resource leaks or clutter. Instead of terminating the program directly, consider logging the error and returning it to the caller, allowing the caller to decide the appropriate course of action, such as retrying the operation, cleaning up resources, or gracefully shutting down the application with proper cleanup.
| func CloneOrCheckout(repo, dir, mirrorDir, primaryBranch, gitopsPath, branchPrefix string) (r *Repo, err error) { | ||
| newRepo := false | ||
| if _, err = os.Stat(dir + "/.git"); os.IsNotExist(err) { | ||
| newRepo = true | ||
| if err = os.MkdirAll(filepath.Dir(dir), os.ModePerm); err != nil && !os.IsExist(err) { | ||
| return nil, err | ||
| } | ||
| if mirrorDir != "" { | ||
| exec.Mustex("", "git", "clone", "-n", "--reference", mirrorDir, repo, dir) | ||
| } else { | ||
| exec.Mustex("", "git", "clone", "-n", repo, dir) | ||
| } | ||
| } else { | ||
| //existing repo | ||
| exec.Mustex(dir, "git", "remote", "set-url", "origin", repo) | ||
| exec.Mustex(dir, "git", "reset", "--hard") | ||
| } | ||
| exec.Mustex(dir, "git", "checkout", "-f", primaryBranch) | ||
| if !newRepo { | ||
| exec.Mustex(dir, "git", "fetch", "origin", "--prune") | ||
| DeleteLocalBranches(dir, branchPrefix) | ||
| } | ||
|
|
||
| return &Repo{ | ||
| Dir: dir, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The function exec.Mustex is used without error handling, which could lead to the program continuing execution even if a critical error occurs during the git operations (e.g., clone, config, checkout). This could result in a partially cloned or misconfigured repository without the user being aware of the failure. It is recommended to handle errors returned by these operations to ensure the integrity of the cloning process and provide clear feedback to the user in case of failure.
| if _, err = os.Stat(dir + "/.git"); os.IsNotExist(err) { | ||
| newRepo = true | ||
| if err = os.MkdirAll(filepath.Dir(dir), os.ModePerm); err != nil && !os.IsExist(err) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
The error handling for os.MkdirAll checks for !os.IsExist(err) which might not be sufficient to catch all relevant errors. Specifically, this condition only checks if the error is not about the directory already existing, but it does not ensure that the directory was successfully created or that it is writable. This could lead to a scenario where the directory creation fails for a reason other than it already existing, but the error is not properly handled, potentially leading to more cryptic errors downstream when trying to use the directory.
A more robust approach would be to handle any error returned by os.MkdirAll directly, without trying to filter it based on its type. If the directory's existence is crucial for the operation to proceed, additional checks should be performed to ensure not only its existence but also its accessibility and suitability for the intended operations.
| SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) | ||
| q="attr(deployment_branch, \".+\", attr(release_branch_prefix, \"${RELEASE_BRANCH}\", kind(gitops, ${TARGET})))" | ||
|
|
||
| targets=$(bazel query ${q} --output label --noshow_progress) |
There was a problem hiding this comment.
The bazel query command uses an unquoted variable ${q}. If the variable contains spaces or special characters, it could lead to unexpected behavior or errors.
Recommended Solution: Quote the variable ${q} to ensure it is correctly interpreted by the bazel query command.
Description
prer should be able to run from a bazel rule. This will allow to skip
bazel buildfor all referred gitops targets. Also, since binary resolving is happening in bazel rule implementation, it is guaranteed to include all required runfiles.Related Issue
#6
Motivation and Context
See #6