Skip to content

Conversation

@caxu-rh
Copy link
Contributor

@caxu-rh caxu-rh commented Jun 13, 2025

This PR addresses #9.

  • Open to alternative naming for the long or short names of the flag. I know of this feature as "pretty-printing" but I suppose it could also be called "formatted" JSON - but I tend to associate --format with accepting a template, e.g. {{ .Foo }} when I want a program (kubectl get, podman inspect) to give me some portion of the JSON output (maybe that could be useful too, but a separate feature).
  • This defaults to sticking with the one-line, unformatted output. We could also switch this to default to multiline, formatted output, and have a flag to choose unformatted (maybe a -r/--raw-output flag instead or something similar).
  • We add a new boolean flag on NewManifestJSONProcessorFn. I considered making this function take a new kind of config/opts struct instead (for future expansion), but I think it could wait for later if/when more options are possible.
  • We currently format with indentation of 4 spaces, not sure if this should be 2, or if it should also be configurable.

@exe-prow-github-app exe-prow-github-app bot requested a review from komish June 13, 2025 14:03
@komish
Copy link
Collaborator

komish commented Jun 13, 2025

@caxu-rh you can just make it pretty by default, and remove the flag altogether. I'm of the "jq exists" mindset, where if someone needed to go from raw --> pretty or vice versa, they could pipe it out to jq.

That said, pretty printing is a reasonable default, but I don't think you need to jump through the hoops to offer both here.

[EDIT] Actually, I'll leave having this be configurable up to your discretion. If you want to leave it, pretty as a default makes sense to me (so make the flag --raw instead)

@komish
Copy link
Collaborator

komish commented Jun 13, 2025

Oh, and if you end up leaving it - feel free to add a config struct as you see fit.

@caxu-rh caxu-rh force-pushed the format-discovery-json branch from 2f5d297 to 86556e8 Compare June 13, 2025 15:04
@caxu-rh
Copy link
Contributor Author

caxu-rh commented Jun 13, 2025

Adjusted to flip the default behavior, forgot to add a config struct - expect another follow-up change.

Signed-off-by: Caleb Xu <caxu@redhat.com>
@caxu-rh caxu-rh force-pushed the format-discovery-json branch from 86556e8 to 7d34dad Compare June 13, 2025 15:25
@caxu-rh
Copy link
Contributor Author

caxu-rh commented Jun 13, 2025

  • Renamed the option to --compact / -c for anyone wanting to keep the compacted output. I figure it is still helpful to keep around in case someone is running this in e.g. some CI flow where some external tools might not be available. I think the naming "compact" is more accurate than "raw" (implies that non-raw output is going through some kind of post-processing, which is not the case) - and I realized that jq's --compact-output option is closer to what we are doing here (--raw-output in jq is also omitting escape chars).
  • Added a new struct type NewManifestJSONProcessorFnOptions - open to naming adjustments. The options in this struct are unique for making a ProcessingFunction that outputs a manifest JSON (not generic to ProcessingFunctions in general). To keep things simple for now, this change includes only the struct (avoiding adding e.g. WithCompactOutput() options-style functions).

Copy link
Collaborator

@komish komish left a comment

Choose a reason for hiding this comment

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

/lgtm

@exe-prow-github-app exe-prow-github-app bot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2025
@exe-prow-github-app
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caxu-rh, komish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@exe-prow-github-app exe-prow-github-app bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2025
@caxu-rh caxu-rh merged commit d7eadc5 into opdev:main Jun 13, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants