Skip to content

Conversation

@cgwalters
Copy link
Collaborator

Mostly just keeping up with things, but this is also prep for us using GetRawBlob.

Mostly just keeping up with things, but this is also prep
for us using `GetRawBlob`.

Signed-off-by: Colin Walters <[email protected]>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The pull request bumps the versions of ocidir and containers-image-proxy. There are several instances where error handling could be improved by explicitly handling the result of functions like read_index and new_empty_manifest instead of unwrapping them. Additionally, adding comments to explain the use of try_clone() would improve code clarity.

)?;

let mut manifest = ocidir::new_empty_manifest().build().unwrap();
let mut manifest = writer.new_empty_manifest()?.build().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider handling the error case where new_empty_manifest fails. Currently, it's unwrapped, which could lead to unexpected program termination.

Suggested change
let mut manifest = writer.new_empty_manifest()?.build().unwrap();
let mut manifest = writer.new_empty_manifest()?;

let idx = tempsrc
.read_index()?
.ok_or(anyhow!("Reading image index from source"))?;
let idx = tempsrc.read_index()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider handling the error case where read_index fails. Currently, the error is not handled, which could lead to unexpected program termination.

Suggested change
let idx = tempsrc.read_index()?;
let idx = tempsrc.read_index()?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gemini is apparently just confused by us dropping a ?, clearly the error is still handled


let mut config = ImageConfigurationBuilder::default().build().unwrap();
let mut manifest = ocidir::new_empty_manifest().build().unwrap();
let mut manifest = self.src_oci.new_empty_manifest()?.build().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider handling the error case where new_empty_manifest fails. Currently, it's unwrapped, which could lead to unexpected program termination.

Suggested change
let mut manifest = self.src_oci.new_empty_manifest()?.build().unwrap();
let mut manifest = self.src_oci.new_empty_manifest()?;

self.src_oci
.replace_with_single_manifest(manifest, oci_image::Platform::default())?;
let idx = self.src_oci.read_index()?.unwrap();
let idx = self.src_oci.read_index()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider handling the error case where read_index fails. Currently, the error is not handled, which could lead to unexpected program termination.

Suggested change
let idx = self.src_oci.read_index()?;
let idx = self.src_oci.read_index()?;

@cgwalters cgwalters enabled auto-merge June 17, 2025 15:02
@cgwalters cgwalters merged commit d72bb14 into bootc-dev:main Jun 17, 2025
32 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants