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 a human readable format to status #738

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

jmarrero
Copy link
Member

supersedes #602 and builds on top of #639

lib/src/status.rs Outdated Show resolved Hide resolved
lib/src/status.rs Outdated Show resolved Hide resolved
@djach7
Copy link
Contributor

djach7 commented Aug 27, 2024

Updated with most recent progress, still in draft status.
Tasks still to do:

  • Format output to be more akin to rpm-ostree status
  • Revise tests to function correctly with all fields
  • Clean up code conventions and style to align with proper bootc/rust conventions

lib/Cargo.toml Outdated Show resolved Hide resolved
lib/src/status.rs Outdated Show resolved Hide resolved
lib/src/status.rs Outdated Show resolved Hide resolved
lib/src/status.rs Outdated Show resolved Hide resolved
lib/src/status.rs Show resolved Hide resolved
@djach7 djach7 force-pushed the human-readable branch 3 times, most recently from e0de226 to a296b12 Compare September 9, 2024 18:31
@djach7 djach7 marked this pull request as ready for review September 9, 2024 18:35
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I think this is getting close. I'd like to consider how to make the output even prettier (I know there's a lot of bikeshedding potential there). Some other projects use emojis (sometimes controversial), but maybe some ascii-art line drawing for associated data (like systemctl status does?)

Image version: stream9.20240807.0 (No timestamp present)
Image transport: registry
Image digest: sha256:47e5ed613a970b6574bfa954ab25bb6e85656552899aa518b5961d9645102b38
No booted image present, checksum defined
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could probably phrase this in a way more understandable to the user. One may very well feel that they are in a "booted image", just not a container image right?

Hmm...how about: "Booted state is native ostree" ?

We could also actually go a step farther and detect the situation of rpm-ostree generated content, though that'd be a good bit more work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the output line to match what you suggested, if you'd like me to dig in further let me know

lib/src/status.rs Outdated Show resolved Hide resolved
lib/src/status.rs Show resolved Hide resolved
lib/src/status.rs Outdated Show resolved Hide resolved
#[test]
fn test_human_readable_base_spec() {
// Tests Staged and Booted, null Rollback
let spec_fixture: &str = include_str!("fixtures/spec-staged-booted.yaml");
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing I think would help here is to factor out a helper function like

fn human_status_from_spec_fixture(&str) -> Result<String> or so

that gets used in each test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented a helper function, Following my interpretation of what you suggested. I used .expect to handled the result, should I instead be using the .unwrap_or that I used to handle the option types?

Implements a human readable output format as the primary
`bootc status` output. Also implements associated testing
using specs from systems in various stages of bootc
deployment.

Co-authored-by: Huijing Hei <hhei@redhat.com>
Co-authored-by: Yasmin de Souza <ydesouza@redhat.com>
Co-authored-by: Steven Presti <spresti@redhat.com>
Co-authored-by: Luke Yang <luyang@redhat.com>
Co-authored-by: Joseph Marrero <jmarrero@redhat.com>

Signed-off-by: djach7 <djachimo@redhat.com>
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Overall looks good! I would like to do some more changes at some point but it's better to do that as followups.

let expected = indoc::indoc! { r"
Current staged image: quay.io/example/someimage:latest
Image version: nightly (2023-10-14 19:22:15 UTC)
Image transport: registry
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about omitting this when it's registry? That's the 95% case I think.

Ok(())
}

fn human_status_from_spec_fixture(spec_fixture: &str) -> Result<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we get an unused function warning?

It may help to put everything here under a

#[cfg(test)]
mod tests {
...
}

as is done elsewhere in the code

@djach7 djach7 merged commit ecabb89 into containers:main Sep 18, 2024
31 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.

3 participants