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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions rust/src/compose.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,12 @@ fn fetch_previous_metadata(
}

#[derive(Debug, Default)]
struct ManifestDiff {
removed: Vec<oci_spec::image::Descriptor>,
added: Vec<oci_spec::image::Descriptor>,
pub struct ManifestDiff {
pub removed: Vec<oci_spec::image::Descriptor>,
pub added: Vec<oci_spec::image::Descriptor>,
}

fn manifest_diff(src: &ImageManifest, dest: &ImageManifest) -> ManifestDiff {
pub fn manifest_diff(src: &ImageManifest, dest: &ImageManifest) -> ManifestDiff {
let src_layers = src
.layers()
.iter()
Expand Down
40 changes: 40 additions & 0 deletions rust/src/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ struct ContainerEncapsulateOpts {
#[clap(long)]
/// Output content metadata as JSON
write_contentmeta_json: Option<Utf8PathBuf>,

/// 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?

}

#[derive(Debug)]
Expand Down Expand Up @@ -215,6 +219,7 @@ fn gv_nevra_to_string(pkg: &glib::Variant) -> String {

/// Like `ostree container encapsulate`, but uses chunks derived from package data.
pub fn container_encapsulate(args: Vec<String>) -> CxxResult<()> {
let org_args = args.clone();
let args = args.iter().skip(1).map(|s| s.as_str());
let opt = ContainerEncapsulateOpts::parse_from(args);
let repo = &ostree_ext::cli::parse_repo(&opt.repo)?;
Expand Down Expand Up @@ -418,6 +423,41 @@ pub fn container_encapsulate(args: Vec<String>) -> CxxResult<()> {
.await
})
})?;

match &opt.compare_with_build {
Some(compare_with_build) => handle.block_on(async {
Comment on lines +427 to +428
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.

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.

let oi_old = proxy
.open_image(compare_with_build)
.await
.expect("open_image");
let (_, manifest_old) = proxy.fetch_manifest(&oi_old).await.expect("fetch_manifest");
let oi_now = proxy
.open_image(org_args.last().expect("arguments").as_str())
.await
.expect("open_image");
let (_, new_manifest) = proxy.fetch_manifest(&oi_now).await.expect("fetch_manifest");
let diff = crate::manifest_diff(&manifest_old, &new_manifest);
let layersum = |layers: &Vec<oci_spec::image::Descriptor>| -> u64 {
layers.iter().map(|layer| layer.size() as u64).sum()
};
let new_total = new_manifest.layers().len();
let new_total_size = glib::format_size(layersum(new_manifest.layers()));
let n_removed = diff.removed.len();
let n_added = diff.added.len();
let removed_size = layersum(&diff.removed);
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.

println!("Removed layers: {n_removed} Size: {removed_size_str}");
println!("Added layers: {n_added} Size: {added_size_str}");
}),
None => (),
};

println!("Pushed digest: {}", digest);
Ok(())
}
Expand Down