-
Notifications
You must be signed in to change notification settings - Fork 92
feat: add support for json output for notation verify
#527
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
Merged
Merged
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
0488b33
Adds additional flag and passes values to notation-go
e0c5113
Fix usage, add error message
eff0563
Merge branch 'notaryproject:main' into attestations-impl
byronchien e9cf5e1
Ignore envelope parsing error and explain in comment
7e1fcd1
use RemoteSignOptions and fix verify error
356a303
Use embedded struct
ed93b36
Add output flag for notation verify
f2004e7
plaintext => text, adds helper for printing objects
f2abaed
Beautify json, add behavior for warnings
f1e85af
Add result to output instead of warnings
cb6d2ed
initialize output object only when output is json
8e3d62e
Merge from main
17f4b82
some nits
b8367b8
Revert the merge but update the error messages for cleaner diff
00d8c1a
Merge branch 'main' into metadata-output
byronchien aa74179
Add e2e test for metadata output
5447ef8
Add SkippedByTrustPolicy result
0ff2455
Update go.mod for notation-go
be0a68a
Merge branch 'main' into metadata-output
byronchien b22ae06
Fix bad merge conflict resolution
d1c6526
Expect failure for E2E metadata test
682d674
Assorted updates
c4a432f
Merge branch 'main' into metadata-output
byronchien 79b3217
Assorted nits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the verification fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for verification failure, no json is written to stdout, and the failure is logged to stderr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@byronchien If JSON is printed only if the verification passes, what's the meaning of showing
"result": "Success"?/cc @priteshbandi @yizha1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When verification fails, I expect that the result would be
failureorfailed, and the failure reason will be included in the JSON object so that other scripts can parse it correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR is merged, I've created #546 to track.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resultcan have two values, one isskipand other one issuccess. IMO for failure case displaying JSON for wont be useful because it wont contain any useful information which automation/script can use(non-zero exit code should suffice to show failure)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSON output is usually consumed by scripts or programs. How can them obtain the structured error message and detailed verification outcomes?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To display error message/detailed verification output we need more than
resultfield something likeresultReasonbecauseresultwill only sayfailure. Also, in current state it wont be of much help since as there is only one genuine/expected failure use case i.e failed signature verification for all the signatures. Apart from genuine/expected error, all other errors should always be emitted as stdErr.Are you suggest we should emit json for expected failure use cases or all failure use cases?
Moved conversation to #546 (comment)