Ap/push updated image same tag #3
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.
Creating two contexts with defer cancel functions in sequence (cancelT and cancel) might lead to confusion and potential misuse in future modifications of the code. It's recommended to combine the context creation and signal notification into a single context to simplify the code and avoid potential errors in future modifications. This can be achieved by using signal.NotifyContext directly with the timeout context, thus maintaining both functionalities within a single context and its corresponding cancel function.
| if _, ok := dstRef.(name.Tag); ok { | ||
| logs.Progress.Printf("fetching manifest for %s", dstRef) | ||
| dst, err := remote.Get(dstRef, remote.WithAuthFromKeychain(authn.DefaultKeychain), remote.WithContext(ctx)) | ||
| if err == nil { | ||
| // if the dst manifest exists, check if it's the same as the src | ||
| if dst.Digest.String() == hash.String() { | ||
| logs.Progress.Printf("found manifest for %s digest %s", dstRef, dst.Digest) | ||
| return nil | ||
| } | ||
| logs.Progress.Printf("manifest digest for %s expected %s got %s", shaDstRef, hash, dst.Digest) | ||
| } |
There was a problem hiding this comment.
The code block that checks if the destination manifest exists and compares it with the source manifest does not handle errors other than a successful fetch (err == nil). If an error occurs during the fetch (other than the manifest not existing), it is silently ignored, which could lead to unintended behavior, especially if the error is due to network issues, authentication problems, or server errors. It's important to handle these errors appropriately to ensure the robustness of the application.
Recommended solution: Introduce error handling for the case where err != nil after attempting to fetch the destination manifest. This could involve logging the error or retrying the fetch operation, depending on the nature of the error.
| _, 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.
Similar to the previous issue, this code block also ignores errors other than a successful fetch when checking if the destination manifest exists for a digest reference. This could lead to the same unintended behavior and lack of robustness in the application.
Recommended solution: Implement error handling for the case where err != nil after attempting to fetch the destination manifest with a digest reference. Consider logging the error or retrying the operation, depending on the error's nature.
| ${REGISTRY_BIN}& | ||
| registry_pid=$! | ||
| trap "kill -9 $registry_pid" EXIT |
There was a problem hiding this comment.
The use of kill -9 to terminate the registry process might be too aggressive and could lead to resource leaks or unclean shutdowns. It's recommended to attempt a graceful shutdown first, using kill without the -9 option, and then fall back to kill -9 only if the process does not terminate after a certain timeout period.
mirror/tests/gitops_setup_test.sh
Outdated
| ${PUSH_IMAGE} | ||
| #setup the enviroment to be reproducible | ||
| K8S_MYNAMESPACE=1 | ||
| export K8S_MYNAMESPACE | ||
|
|
||
| #FIXME: generated .apply uses system kubectl so we need to fake it | ||
| kubectl_path=$(dirname ${KUBECTL}) | ||
| export PATH=${kubectl_path}:${PATH} | ||
|
|
||
| echo executing test setup script $1 | ||
| $1 | ||
| if [ $? -ne 0 ]; then | ||
| echo "Test setup script failed" | ||
| exit 1 | ||
| fi | ||
| echo verifying image $2 | ||
| ${CRANE_BIN} validate -v --fast --remote $2 |
There was a problem hiding this comment.
The script does not check the success of the ${PUSH_IMAGE} command on line 10, which could lead to false positives in the test if the image push fails but subsequent commands succeed. It's important to check the exit status of each critical command in a script to ensure that the script fails fast and provides accurate feedback on where the error occurred. Adding an exit status check after the ${PUSH_IMAGE} command would improve the robustness of the script.
mirror/tests/kubectl
Outdated
| if [[ $1 == "apply" ]]; then | ||
| echo "faking kubectl apply -f " | ||
| cat >/dev/null | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
The script is designed to intercept kubectl apply commands and simulate their behavior by doing nothing and exiting with a success status. However, this approach has a couple of issues:
-
Security Concern: The script writes input directly to
/dev/nullwithout any validation or sanitization. While in this specific context (writing to/dev/null) it might not pose a direct security risk, adopting such practices can lead to security vulnerabilities when applied in different contexts. It's essential to validate and sanitize input in scripts to prevent potential security issues. -
Lack of Flexibility and Feedback: The script provides no feedback or logging mechanism for the intercepted commands, which could be useful for debugging or auditing purposes. Additionally, it only handles the
applycommand and exits for any other commands, which might not be the desired behavior in all testing scenarios.
Recommended Solution:
- Implement input validation and sanitization to ensure that the script handles input safely. Even though it might not be strictly necessary for writing to
/dev/null, it's a good practice to follow. - Consider adding logging or feedback mechanisms to provide insight into the intercepted commands and actions taken by the script. This could be beneficial for debugging and understanding the script's behavior during tests.
- Evaluate the need to handle other
kubectlcommands or provide a more flexible way to simulate different behaviors based on the input commands. This could make the script more versatile and useful in a broader range of testing scenarios.
| bash -x $1 | ||
| if [ $? -ne 0 ]; then | ||
| echo "Test setup script failed" | ||
| exit 1 |
There was a problem hiding this comment.
The script uses a direct check of $? to determine the success of the bash -x $1 command, which can be error-prone and less readable, especially when the script grows in complexity. Instead of directly checking $?, it's recommended to use set -e at the beginning of the script to ensure that the script exits immediately if any command exits with a non-zero status. This approach simplifies error handling and makes the script more robust.
check sha of the dst image if tag is used to make sure mirror is happening if image is updated without changing the tag.
Also, resolves #2