-
Notifications
You must be signed in to change notification settings - Fork 645
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
Add audit interface for validation API #712
Conversation
Fix #686. Each content validation function should return both hard error and warned errors, like: ``` func validateDescriptor(r io.Reader) ([]error, error) ``` Add a `ValidationAudit` interface on Validate API argument. This interface allows consumer to implement `Warns` method, to decide how to handle warned errors. Signed-off-by: xiekeyang <xiekeyang@huawei.com>
@xiekeyang I am not sure that having methods return |
@stevvooe I had read the comment, and guess you want consumer implement In your comment: if err := validate(content); err != nil {
for _, warning := range Warnings(err) {
// do something with warnings
}
} I feel it is a little unclear that you likely want to exec customized methods (Warnings) out of API, and input arg is just If above understand is right, content validation will be broken when it encounter a warn. That might impact consumer badly even he doesn't care warn. in my implementation, API and its mapped-functions(validateManifest, validateDescriptor...) determine which is hard error(unconditional break function), which are warns(just throw to consumer). If consumer care warn, it can also break validation with Maybe I miss your point, please point it, and make some more description on your code segment. I would figure out a better way. |
@xiekeyang The goal here is to treat all validation problems as errors. If a user wants to opt out, they can. This API prevents complexity for the common case. Please follow it, as designed. The validation code is really broken as of 1.0, so I don't think breaking is a problem. |
@stevvooe While I still a little misunderstand, here I guess you want: 1:validMediatypeMap returns func validateDescriptor(r io.Reader) []error func validateConfig(r io.Reader) []error func validateManifest(r io.Reader) []error 2:And we define type Warnings interface{
Warnings([]error) error
}
type Errors interface{
Errors([]error) error
} Then customized methods can be done inside validation API: func (v Validator) Validate(src io.Reader, warns Warnings, errors Errors) error{
...
errs := validateMediatypeMap(src)
err := warns.Warnings(errs)
err = errors.Errors(errs)
...
} Above design makes sure that:
If you are unsatisfied with above, then please help me to clear some problems about your issue comment, and I would amend: Problem 1if err := validate(content); err != nil {
for _, warning := range Warnings(err) {
// do something with warnings
}
}
if err := validate(content); err != nil && IgnoreWarnings(err) {
// handle hard errors
}
func validate(content) error{
err:=validMediatypeMap(content);
err=gojsonschema.Validate()
...
return err
} It seems a weak function. Problem 2And for this code line: Problem 3type Warnings {
Warnings() []error
}
type Errors {
Errors() []error
} If validation API just need validate problems and return 1 error, why it need define interface? |
I'm going to close this PR in favor of discussing design on #686. |
Fix #686.
Each content validation function should return both hard error and
warned errors, like:
Add a
ValidationAudit
interface on Validate API argument. Thisinterface allows consumer to implement
Warns
method, to decide how tohandle warned errors. like:
Signed-off-by: xiekeyang xiekeyang@huawei.com
ping @stevvooe @vbatts If it is on the right way?