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.

Sign up for GitHub

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

Jump to bottom

imagefilter: adjust short output format #1206

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

imagefilter: adjust short output format #1206

wants to merge 1 commit into from
+22 −20

Conversation

schuellerf
Copy link
Contributor

Adjusts the output format to match the cloudAPI /distributions See: osbuild/osbuild-composer#4336
Now this is considered "stable"?

The output is not as nice as before but ok-ish and consistent with the API, now.

centos-9:
 aarch64: [ ami, edge-ami, edge-commit, edge-container, edge-installer, edge-raw-image, edge-simplified-installer, edge-vsphere, image-installer, minimal-raw, openstack, qcow2, tar, vhd, wsl ]
 ppc64le: [ qcow2, tar ]
 s390x: [ qcow2, tar ]
 x86_64: [ ami, edge-ami, edge-commit, edge-container, edge-installer, edge-raw-image, edge-simplified-installer, edge-vsphere, gce, image-installer, minimal-raw, oci, openstack, ova, qcow2, tar, vhd, vmdk, wsl ]
centos-10:
 aarch64: [ ami, image-installer, qcow2, tar, vhd, wsl ]
 ppc64le: [ qcow2, tar ]
…

@schuellerf schuellerf requested a review from a team as a code owner February 10, 2025 16:55
supakeen
supakeen reviewed Feb 10, 2025
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm uncomfortable about string formatting all this YAML.

@supakeen
Copy link
Member

As a side question, is the stability goal here to be equal to the structures as output by osbuild-composer's CloudAPI?

@schuellerf
imagefilter: adjust short output format
c7aaad0 
Adjusts the output format to match the cloudAPI `/distributions`
See: osbuild/osbuild-composer#4336
Now this is considered "stable".
@schuellerf schuellerf force-pushed the adjust-short-output-format branch from 37f4c4f to c7aaad0 Compare February 11, 2025 09:21
@schuellerf
Copy link
Contributor Author

@supakeen it was a discussion essentially started here where it almost looks yaml-like so we just made it to be valid.
I'll check if it's worth introducing a dependency for this.

As also noted in the comment of the warning (which got removed with this PR), the "instability" was due to misalignment with osbuild/osbuild-composer#4336 which is fixed now.

@supakeen
Copy link
Member

@schuellerf The YAML dependency will likely be there soon due to: #1205 :)

mvo5
mvo5 requested changes Feb 11, 2025
View reviewed changes
pkg/imagefilter/formatter.go
// are open questions, e.g. how this relates to:
// https://github.com/osbuild/osbuild-composer/pull/4336
// which adds a similar but slightly different API
fmt.Fprint(w, "@WARNING - the output format is not stable yet and may change\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to keep this for now, I think we need to consider what API we provide and grantee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I'll think about another code-comment as it's aligned with the 4336-PR…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

…and it doesn't state it's yaml, just happens to be compatible for now ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't say a warning on the cloud-api though (and it's essentially the same output)
Should I transition to yaml library and remove the warning or make it really short and not making it yaml at all?
@supakeen / @mvo5

Copy link
Member

Choose a reason for hiding this comment

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

So, with #1205 landing there's no reason anymore not to pull in a YAML dependency as it's already here.

My preference is that if the output format is outputting YAML that the format should be called yaml as well, same goes for the JSON format we have.

That means we can either, remove the YAML-ness in this PR and name it short or we can make it go full-YAML and name it yaml. No real preference on either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just pushes me towards making both :-D
I'm fine with that ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supakeen supakeen supakeen left review comments

@mvo5 mvo5 mvo5 requested changes

@thozza thozza Awaiting requested review from thozza thozza is a code owner automatically assigned from osbuild/osbuild-reviewers

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@schuellerf @supakeen @mvo5