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

Fix verify and verify-attestation output flag #546

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

josedonizetti
Copy link
Contributor

@josedonizetti josedonizetti commented Aug 15, 2021

There are a few things on this PR.

1 - improve documentation, listing the possible options for -output -> '(json|text)'
2 - fix existing behavior, where -output is actually ignored, using always text, even though the documentation says the default is json.

A couple questions:

1 - I think it makes more sense to rename this flag to -format, instead of -output considering the options (json/text).
2 - The current default is text, but fixing this to follow the documentation, it becomes json, but I think text looks better on the command line.

wdyt?

Signed-off-by: Jose Donizetti <jdbjunior@gmail.com>
@dlorenc
Copy link
Member

dlorenc commented Aug 15, 2021

1 - I think it makes more sense to rename this flag to -format, instead of -output considering the options (json/text).

Could we keep output as an alias? I think you're right, but would prefer not to break stability right after 1.0 :(

@josedonizetti
Copy link
Contributor Author

@dlorenc What do you think about merging as is? It fixes the hardcoded option, allowing the user to choose a format, and I can follow up with a different PR for a new format flag alias of output.

@developer-guy
Copy link
Member

@dlorenc What do you think about merging as is? It fixes the hardcoded option, allowing the user to choose a format, and I can follow up with a different PR for a new format flag alias of output.

yeah make sense to me, I'll approve the PR as is, as you said maybe you can create another PR for the alias thing, PTAL @dlorenc

Copy link
Member

@developer-guy developer-guy left a comment

Choose a reason for hiding this comment

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

LGTM

@dlorenc dlorenc merged commit 3733e69 into sigstore:main Aug 20, 2021
@josedonizetti josedonizetti deleted the fix-output-flag branch August 20, 2021 11:45
@cpanato cpanato added this to the v1.1.0 milestone Aug 20, 2021
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.

4 participants