-
Notifications
You must be signed in to change notification settings - Fork 7
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!: return CertificateVerficationResponse{} in VerifyCert() #74
Conversation
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'm fine doing this change to the API
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'm fine with the change. Just take a look on Flavio's comments.
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.
once comments are addressed
pkg/capabilities/VerifyCert() returns a `(bool, err)`. In the case that it returns `(false, err)`, the error is not a runtime error but it may contain the reason for the failed certificate verification. Return a ``(CertificateVerficationResponse{},err)` instead, which contains the bool trusted and a reason, or a runtime error. Signed-off-by: Víctor Cuadrado Juan <[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
Description
Relates to kubewarden/cel-policy#15.
pkg/capabilities/VerifyCert()
returns a(bool, err)
. In the case that it returns(false, err)
, the error is not a runtime error but it may contain the reason for the failed certificate verification.Return a
(CertificateVerficationResponse{},err)
instead, which contains the bool trusted and a reason, or a runtime error.Test
CI.
Additional Information
Tradeoff
Potential improvement