-
Notifications
You must be signed in to change notification settings - Fork 42
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: update error message from notation-go #345
Conversation
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #345 +/- ##
==========================================
- Coverage 74.68% 74.36% -0.32%
==========================================
Files 23 24 +1
Lines 2228 2251 +23
==========================================
+ Hits 1664 1674 +10
- Misses 443 457 +14
+ Partials 121 120 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
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.
LGTM
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
- Update oras-go to v2.3.0. - Replace oras.Pack() with oras.PackManifest() as it is deprecated in v2.3.0. - Generate an empty config blob manually, as oras.PackManifest() does not generate the config blob with the notation artifact type as the media type. Resolves notaryproject#346 Signed-off-by: Junjie Gao <[email protected]> --------- Signed-off-by: Junjie Gao <[email protected]>
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.
LGTM with suggestions
Signed-off-by: Patrick Zheng <[email protected]>
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.
LGTM
Couple of observations:
|
@priteshbandi
I have replied this one in: notaryproject/notation#771 (comment) |
Hi @priteshbandi, regarding your concern on returning errors for all the signatures, this won't bring in endless attack: https://github.com/notaryproject/notation-go/blob/main/notation.go#L361, because we use |
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.
LGTM
Signed-off-by: Patrick Zheng <[email protected]>
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.
LGTM
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.
LGTM
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.
LGTM
This PR tends to solve the following issues from Notation CLI which requires a few changes in notation-go library:
#699, #700, #701.
This PR includes:
UpdatedverificationOutcomes
in the return ofnotation.Verify
to include verification failed reasons of each signature, so that Notation CLI could display them to the user without having to enable the-v
or-d
flag.err
returned fromnotation.Verify
as a joined error. (based on code review,verificationOutcomes
related logic is not changed in this PR.)The updated error messages is displayed in the PR of Notation CLI: notaryproject/notation#771.