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

cli: Add JSON output option for pull --check & compare #618

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

travier
Copy link
Member

@travier travier commented Apr 16, 2024

This will make it easier for other higher level tools (Plasma Discover
for example) to read the output of those commands.

@travier
Copy link
Member Author

travier commented Apr 16, 2024

Draft as we probably want to use a JSON crate for this.

@travier travier force-pushed the main-manifest-diff-json branch 2 times, most recently from 01058c4 to a131591 Compare April 17, 2024 11:37
@travier travier marked this pull request as ready for review April 17, 2024 11:37
@travier
Copy link
Member Author

travier commented Apr 17, 2024

c9s test is failing because it tries to build bootc and thus needs containers/bootc#472.

Maybe I should not change the number of args of this function and add a new print_json one instead?

@cgwalters
Copy link
Member

Maybe I should not change the number of args of this function and add a new print_json one instead?

Right, we can't change public API without bumping the semantic version. (Which is OK to do, but then we'd need to turn off the reverse-dependency build checks temporarily)

println!("Added layers: {print_n_added:<4} Size: {print_added_size}");
pub fn print(&self, json: bool) {
if json {
let out = json!({
Copy link
Member

@cgwalters cgwalters Apr 17, 2024

Choose a reason for hiding this comment

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

Instead of this we can add #[derive(Serialize)] for ManifestDiff and get these bits for free. We'll likely need to skip serializing the held references.

And as a bonus, that's a fully API compatible change that wouldn't require changing print.

@cgwalters
Copy link
Member

Closing in favor of #619

This will make it easier for other higher level tools (Plasma Discover
for example) to read the output of those commands.
@travier travier changed the title container: Support JSON out for ManifestDiff cli: Add JSON output option for pull --check & compare Apr 17, 2024
@@ -234,6 +239,10 @@ pub(crate) enum ContainerImageOpts {
/// the new manifest.
#[clap(long)]
check: Option<Utf8PathBuf>,
Copy link
Member Author

@travier travier Apr 17, 2024

Choose a reason for hiding this comment

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

It looks like this Option is not respected by clap and always asked on the command line:

$ ostree container image pull --check check repo ostree-image-signed:registry:a.a/a:a
error: Opening directory at 'repo': No such file or directory (os error 2)

$ ostree container image pull --check repo ostree-image-signed:registry:a.a/a:a
error: the following required arguments were not provided:
  <IMGREF>

Usage: ostree-container container image pull --check <CHECK> <REPO> <IMGREF>

For more information, try '--help'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, OK, it's a little bit more complex than that for the pull --check option as it optionally writes to a file.

@travier travier marked this pull request as draft April 17, 2024 14:39
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