merge rules_mirror in #37
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| ctx, cancelT := context.WithTimeout(context.Background(), Timeout) | ||
| defer cancelT() | ||
| ctx, cancel := signal.NotifyContext(ctx, os.Interrupt) | ||
| defer cancel() |
There was a problem hiding this comment.
The context is being wrapped twice: first with a timeout and then with a signal notification. This can lead to confusion and potential issues with context cancellation. Instead, combine both functionalities into a single context to simplify the logic and ensure that the context is managed consistently.
| if err := mirror.ExecuteContext(ctx, FromLocation, To, Digest); err != nil { | ||
| logs.Warn.Printf("Failed to mirror %s to %s: %v", FromLocation, To, err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
The error handling in the mirror.ExecuteContext function only logs a warning and exits with a status code of 1. This approach does not provide detailed information about the failure. Consider enhancing the error handling to include more context or to perform additional cleanup actions if necessary.
| if [ -d ${BASH_SOURCE[0]}.runfiles ]; then | ||
| echo "$( cd ${BASH_SOURCE[0]}.runfiles && pwd )" |
There was a problem hiding this comment.
Using ${BASH_SOURCE[0]} without quotes can lead to issues if the script path contains spaces or special characters. This can cause the script to fail unexpectedly.
Recommended Solution:
Always quote variable expansions to prevent word splitting and globbing issues.
|
|
||
| RUNFILES="${PYTHON_RUNFILES:-$(guess_runfiles)}" | ||
|
|
||
| {mirror_tool} -from {src_image} -digest {digest} -to {dst_image} -timeout {timeout} |
There was a problem hiding this comment.
The script directly executes the {mirror_tool} command without any error handling. If the command fails, the script will terminate due to set -e, but no error message will be provided to the user.
Recommended Solution:
Add error handling around the command execution to provide a meaningful error message if the command fails.
| func ExecuteContext(ctx context.Context, fromLocation, to, digest string) error { | ||
| fromRef, err := name.ParseReference(fromLocation) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| logs.Debug.Printf("in: %s/%s:%s", fromRef.Context().RegistryStr(), fromRef.Context().RepositoryStr(), fromRef.Identifier()) | ||
| dstRef, err := name.ParseReference(to) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| logs.Debug.Print("out:", dstRef) | ||
| hash, err := v1.NewHash(digest) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ref, err := name.NewDigest(fmt.Sprintf("%s@%s", fromRef.Context(), hash.String())) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| shadst := fmt.Sprintf("%s@%s", dstRef.Context(), hash.String()) | ||
| shaDstRef, err := name.NewDigest(shadst) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // check if dst exists already. | ||
| // We are not verifying if the dst has the same tag | ||
| logs.Progress.Printf("fetching manifest for %s", shaDstRef) | ||
| _, err = remote.Head(shaDstRef, remote.WithAuthFromKeychain(authn.DefaultKeychain), remote.WithContext(ctx)) | ||
| if err == nil { | ||
| // if the dst manifest exists, check if it's the same as the src | ||
| logs.Progress.Printf("found manifest for %s", shaDstRef) | ||
| return nil |
There was a problem hiding this comment.
The code does not handle the case where the remote.Head request fails due to reasons other than the manifest not existing (e.g., network issues, authentication problems). This could lead to the function incorrectly assuming the destination manifest does not exist and proceeding to fetch and push the source manifest unnecessarily.
Recommended Solution:
Add error handling to differentiate between a non-existent manifest and other types of errors. If the error is not a 404, handle it appropriately (e.g., by returning the error).
| if got := os.Getenv("TEST_OCI_REGISTRY"); got != "" { | ||
| reg, err := name.NewRegistry(got) | ||
| if err != nil { | ||
| t.Fatalf("failed to parse TEST_OCI_REGISTRY: %v", err) | ||
| } | ||
| return reg, func() {} |
There was a problem hiding this comment.
The function SetupRegistry does not handle the case where name.NewRegistry returns an error gracefully when TEST_OCI_REGISTRY is set. While it does log the error and fail the test, it might be more useful to provide additional context or suggestions for resolving the issue, especially since environment variables can often be misconfigured.
Recommended Solution:
Enhance the error message to include possible reasons for the failure and steps to resolve it, such as checking the format of the TEST_OCI_REGISTRY environment variable.
| ${REGISTRY_BIN}& | ||
| registry_pid=$! | ||
| trap "kill -9 $registry_pid" EXIT |
There was a problem hiding this comment.
The script uses kill -9 to terminate the registry process. This is a forceful termination and does not allow the process to clean up resources properly. It is generally better to use a gentler signal like SIGTERM (kill -15) to allow the process to exit gracefully.
Recommended Solution: Replace kill -9 with kill -15 to allow the registry process to terminate gracefully.
| bash -x $1 | ||
| if [ $? -ne 0 ]; then | ||
| echo "Test setup script failed" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The script runs the test setup script and checks its exit status, but it does not capture or log the output of the script, which can be crucial for debugging if the script fails.
Recommended Solution: Capture the output of the test setup script and log it to a file or display it in the console for easier debugging in case of failure.
| if [[ $1 == "apply" ]]; then | ||
| echo "faking kubectl apply -f " | ||
| cat >/dev/null | ||
| exit 0 | ||
| else | ||
| echo "dummy" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
The script does not handle unexpected arguments or errors. If an argument other than "apply" is passed, it simply echoes "dummy" and exits with a status of 0. This can lead to confusion and make debugging difficult.
Recommended Solution:
- Add an
elseclause to handle unexpected arguments and exit with a non-zero status code to indicate an error. This will make the script more robust and easier to debug.
| @@ -0,0 +1,3 @@ | |||
| #!/bin/bash | |||
| echo "Validating image $1" | |||
| $CRANE_BIN validate -v --fast --remote $1 | |||
There was a problem hiding this comment.
The script does not handle the case where $CRANE_BIN is not set or is set to an invalid path. This could lead to a confusing error message or failure to execute the validation.
Recommended Solution:
Add a check to ensure that $CRANE_BIN is set and points to a valid executable before attempting to use it. If it is not set or invalid, print an error message and exit the script with a non-zero status.
No description provided.