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 short output #1167

Merged
merged 2 commits into from
Jan 27, 2025
Merged

Add short output #1167

merged 2 commits into from
Jan 27, 2025

Conversation

schuellerf
Copy link
Contributor

@schuellerf schuellerf commented Jan 24, 2025

Add short output
which looks like

rhel-9.5:
  ami: [ aarch64, x86_64 ]
  azure-rhui: [ aarch64, x86_64 ]
  azure-sap-rhui: [ x86_64 ]
  ec2: [ aarch64, x86_64 ]
…
rhel-9.6:
  ami: [ aarch64, x86_64 ]
  azure-rhui: [ aarch64, x86_64 ]
  azure-sap-rhui: [ x86_64 ]
  ec2: [ aarch64, x86_64 ]
  ec2-ha: [ x86_64 ]
  ec2-sap: [ x86_64 ]

(this is based on #1166 )

@schuellerf schuellerf requested a review from a team as a code owner January 24, 2025 16:11
@schuellerf schuellerf marked this pull request as draft January 24, 2025 16:18
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This is an interesting idea, what if we would make it valid yaml so that it would look nice but still be (relatively) easy to parse? How would that look like? Note that I don't think we need a fully yaml parser/struct here (at least initially) to test the idea, just re-structuing the output that its a valid yaml map if distroname that then has keys with type and a list of arches (which is super close to what is written here already). Wdyt?

@schuellerf schuellerf force-pushed the add-short-output branch 3 times, most recently from a14bf79 to 236403a Compare January 24, 2025 16:29
@schuellerf
Copy link
Contributor Author

I added a colon after the distro, so now technically it's yaml but the arches are no list :-/
I'll think about it if making this a valid yaml list hurts readability too much…

Fixed spellchecking by changing to `code` as this is
accepted and also more valid semantically.
The 'short' output format should provide a better overview
when looking at all combinations.
@schuellerf
Copy link
Contributor Author

schuellerf commented Jan 27, 2025

Thank you @mvo5 for your hint, it seems possible to have this a valid yaml.
I updated the description accordingly, please check.

The structure does not make too much sense, I guess (to nest distro -> type -> arch) but still you can parse it if needed and it's well readable which is the main goal of this output format.

@schuellerf schuellerf marked this pull request as ready for review January 27, 2025 15:14
@schuellerf
Copy link
Contributor Author

btw I tested the output with:

./bin/image-builder list-images --output short | yq -P .

@schuellerf schuellerf requested a review from mvo5 January 27, 2025 17:02
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Thank you.

Some remarks but non-blocking:

  • Should we instead call this yaml if we're committed to making it YAML?
  • In that case, should we 'just' have a proper yaml encoder just like json?

I don't particularly like the spellcheck commit showing up in this PR but if it blocks CI otherwise, I guess.

@schuellerf
Copy link
Contributor Author

For yaml you would probably expect the data structure to be the same as for json (just being a list of "objects").
In this case the main use case is really to get a short output. But if we decide otherwise or want yaml additionally, I'd be happy to implement in a followup PR 👌

@schuellerf schuellerf added this pull request to the merge queue Jan 27, 2025
Merged via the queue into osbuild:main with commit aa79e76 Jan 27, 2025
19 checks passed
@schuellerf schuellerf deleted the add-short-output branch January 27, 2025 18:44
@mvo5
Copy link
Contributor

mvo5 commented Jan 28, 2025

Fwiw, I think this got merged a bit too quickly, I think there are still open questions like how exactly this should look like (and I would have liked to review the new implementation too). There is also the open question of how this relates to https://github.com/osbuild/osbuild-composer/pull/4336/files#diff-792fae8eca7351040416f58a710aa554fe76d91e6f39cfbe122bf85428f34afdR572 - which is similar but slightly different which is now confusing for users of our API.

@schuellerf
Copy link
Contributor Author

Sorry, I thought all your remarks have been addressed :-/
We can for sure align here and I'll do a followup!

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