Skip to content
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

Add --compare-with-build to cli #4294

Closed
wants to merge 1 commit into from

Conversation

RishabhSaini
Copy link
Contributor

This is a prep PR to completing #4247. It allows one to diff the layers of current encapsulated build to any other build.

This is a prep PR to completing coreos#4247.
It allows one to diff the layers of current encapsulated build to any other build.
@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Feb 9, 2023

This PR is there to look at the diff in the size of layers between two OCI images. This would be helpful in testing as I need to find which algorithm performs better.

Some(compare_with_build) => handle.block_on(async {
let proxy = containers_image_proxy::ImageProxy::new()
.await
.expect("c/img-proxy");
Copy link
Member

Choose a reason for hiding this comment

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

This should use ? here and below.

Comment on lines +427 to +428
match &opt.compare_with_build {
Some(compare_with_build) => handle.block_on(async {
Copy link
Member

Choose a reason for hiding this comment

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

This is more succinct as if let Some(compare_with_build) = opt.compare_with_build.as_ref() {

Copy link
Member

Choose a reason for hiding this comment

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

Also, let's split the handling here into a separate function.


/// Compare OCI layers of current build with another(imgref)
#[clap(name = "compare-with-build", long, short)]
compare_with_build: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is more convenient as a separate CLI command that can take two arbitrary builds? i.e. rpm-ostree container compare-images? Or hmm, I guess if we're keeping it independent of RPM content it could live in ostree even as ostree container compare-images or so?

let removed_size_str = glib::format_size(removed_size);
let added_size = layersum(&diff.removed);
let added_size_str = glib::format_size(added_size);
println!("Total new layers: {new_total} Size: {new_total_size}");
Copy link
Member

Choose a reason for hiding this comment

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

Would also make sense to have a handy fn print(&self) on ManifestDiff I think so we can share this code with the compose path.

@RishabhSaini
Copy link
Contributor Author

ostree container compare-images makes more sense to me. Will transfer the functionality there with the review changes.

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.

None yet

2 participants