-
Notifications
You must be signed in to change notification settings - Fork 62
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
refactor: refactor error handling #956
Conversation
Thanks for this quality work @binbin-li! Would it be possible to get a screenshot of what the new formatted logs look like? Same for the error message from GK and the CLI? |
191cbb2
to
24e71bd
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #956 +/- ##
==========================================
+ Coverage 57.45% 58.17% +0.72%
==========================================
Files 90 91 +1
Lines 5326 5447 +121
==========================================
+ Hits 3060 3169 +109
- Misses 1956 1964 +8
- Partials 310 314 +4
☔ View full report in Codecov by Sentry. |
24e71bd
to
45d401b
Compare
45d401b
to
dea70a6
Compare
b665971
to
88815db
Compare
thanks the sample errors in the images. Just to clarify , when do we add stack trace to errors ? stack trace is needed when something is wrong ( ratify code error) , i want to make sure when Ratify successfully run validation,we don't pass a stack trace on a bad image. Also if it is a configuration error, customer need to fix the configuration, but they won't need to investigate the code base |
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.
this is a great improvement @binbin-li over our current message, left some comments.
a3cffef
to
67c7a62
Compare
67c7a62
to
535b0a0
Compare
d808740
to
6c29575
Compare
c7ea846
to
3ee7cf3
Compare
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.
Looks great! lets sync on the TSG
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.
Looks great! lets sync on the TSG
2c5f14a
to
b70b136
Compare
b70b136
to
aeb78ca
Compare
synced offline. thanks a lot for feedback @susanshi |
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. thanks!
fix: fix cert-rotator test (ratify-project#992) chore: bump github.com/aws/aws-sdk-go-v2/config from 1.18.32 to 1.18.33 (ratify-project#988) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Akash Singhal <akashsinghal@microsoft.com> feat: add graceful shutdown for http server (ratify-project#949) fix: Updating akv cert provider to use getSecret (ratify-project#957) Signed-off-by: Susan Shi <huish@microsoft.com> chore: bump golangci/golangci-lint-action from 3.6.0 to 3.7.0 (ratify-project#997) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore: bump actions/setup-go from 4.0.1 to 4.1.0 (ratify-project#996) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> fix: adding experimental to dynamic plugin flag (ratify-project#980) refactor: refactor error handling (ratify-project#956) docs: add notaryv2 upgrade doc (ratify-project#999) chore: update assign.yaml template chore: update library templates chore: update library templates2 chore: update library templates fix typo chore: update library templates array
Description
What this PR does / why we need it:
Design doc: https://hackmd.io/@H7a8_rG4SuaKwzu4NLT-9Q/HJ6bNe2c2
The exported error codes could be checked on pkg.go.dev, just like this: https://pkg.go.dev/github.com/distribution/distribution@v2.8.2+incompatible/registry/api/errcode#pkg-variables
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when the PR gets merged):Fixes #856
Type of change
Please delete options that are not relevant.
main
branch)How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
I tested the error format in both add-on service and CLI tool.
Add-on service.
The service log would look like:
CLI.
Checklist:
Post Merge Requirements
Helm Chart Change