-
Notifications
You must be signed in to change notification settings - Fork 58
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
Refactoring error messages to use %w
formatting directive and fix logging issue
#314
Conversation
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
…nto improving-errors
%w
formatting directive and fix logging issue
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
cmd/config.go
Outdated
err := fmt.Errorf("failed to set config value: %w", err) | ||
log.Error(err) |
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.
Same question as the other PR, any reason to not have this as a single line with log.Errorf()
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.
Hey 👋 - I answered in the other PR: in-toto/go-witness#83 (comment)
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.
I went and checked out the other PR but didn't see an explanation for this line break?
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.
sorry if this was all very unclear. The reason for the splitting out the error formatting to fmt.Errorf
is because log.Errorf
does not support the %w
directive. The reason for not having it all on a single line (e.g., log.Errorf(fmt.Errorf(...
was for ease of readability.
After being round the houses with all of this a little bit, I believe we can switch all this to a log.WithError(err).Error(...
. I will push said changes to this PR 😄
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.
Thanks for the contribution!!
Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>
…nto improving-errors
logs are not printing correctly after these changes, but this is just because the fix is made in in-toto/go-witness#85 and therefore won't be utilised in This has been tested against
|
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 good!
When looking at #293, it was clear that there were some issues occurring with formatting of error messages:
ERROR failed to create fulcio signer: %!w(*errors.errorString=&{square/go-jose: missing payload in JWS message})
After exploring this, it seemed that using
fmt.Errorf
to format the error first was the safer way to go, and using the%w
formatting directive was worthwhile as it gives returned errors anUnwrap
method for unwrapping the argument presented with%w
initially (see here).The result of the refactoring gives the following error message:
ERROR failed to create fulcio signer: square/go-jose: compact JWS format must have three parts
This PR coincides with PR in-toto/go-witness#83 created in the
https://github.com/in-toto/go-witness
repository.Note: This is a proposed change to the structure. If anyone wishes to suggest a better way of structuring this I am all ears!