-
Notifications
You must be signed in to change notification settings - Fork 211
rechunker: Use open_image when transport is oci-archive #5445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request modifies the logic for checking existing OCI images to better handle the oci-archive transport, especially when the archive file doesn't exist. The change uses proxy.open_image instead of proxy.open_image_optional for this transport. A new test case is added to test.sh to verify this functionality. The review suggests improving error handling in the Rust code and ensuring test hygiene in the shell script.
| let img = if transport == OCI_ARCHIVE_TRANSPORT { | ||
| (proxy.open_image(output).await).ok() | ||
| } else { | ||
| proxy.open_image_optional(output).await? | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using .ok() here swallows errors from proxy.open_image().await, treating any failure as if the image does not exist. This could mask underlying issues such as permissions errors or invalid paths, leading to unexpected behavior.
It would be more robust to handle only the "not found" case as a non-error condition and propagate other errors. At a minimum, logging the discarded error at a debug level would aid in troubleshooting potential issues in the future.
let img = if transport == OCI_ARCHIVE_TRANSPORT {
match proxy.open_image(output).await {
Ok(img) => Some(img),
Err(e) => {
tracing::debug!("Ignoring error opening oci-archive: {e}");
None
}
}
} else {
proxy.open_image_optional(output).await?
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this as a followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a helper for this bootc-dev/bootc#1464
This allows the use of oci-archive when the archive doesn't yet exist. Signed-off-by: ckyrouac <[email protected]>
388b508 to
ac4b320
Compare
|
/override ci/prow/fcos-e2e |
|
@cgwalters: Overrode contexts on behalf of cgwalters: ci/prow/fcos-e2e In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This allows the use of oci-archive when the archive doesn't yet exist.
fixes #5442