cmd/oci-runtime-tool: Implement --compliance-level #492
Conversation
The man-page entry and Bash completions for this option were added in 4029999 (oci error: add error level and reference, 2017-03-31, opencontainers#354), but the option was left unimplemented. The implementation in this commit is very similar to the existing runtimetest implementation from 4029999, except we warn instead of silently skipping non-fatal spec violations. The warning (vs. error) on invalid level arguments follows the runtimetest implementation from 6316a4e (use released version as reference; improve Parse error, 2017-04-12, opencontainers#354). I'd personally prefer hard errors on invalid level arguments, but didn't want to break runtime-tools consistency over that. Signed-off-by: W. Trevor King <[email protected]>
cmd/oci-runtime-tool/validate.go
Outdated
| @@ -27,8 +37,20 @@ var bundleValidateCommand = cli.Command{ | |||
| } | |||
|
|
|||
| if err := v.CheckAll(); err != nil { | |||
| return err | |||
|
|
|||
| merr, ok := err.(*multierror.Error) | |||
There was a problem hiding this comment.
It'd be nice if this was a helper function we could reuse in tests.
This doesn't save much time for folks who are iterating over the removed errors (e.g. to print warnings about them), but Aleksa asked for it [1]. I haven't used it in runtimetest. I'm waiting for the in-flight 7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors, 2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the runtimetest error-processing code with: validations := defaultValidations if platform == "linux" { validations = append(validations, linuxValidations...) } and a single loop over the constructed validations array. [1]: opencontainers#492 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
force-pushed
the
validate-compliance-level
branch
from
91aa0e9 to
be12cce
Compare
This doesn't save much time for folks who are iterating over the removed errors (e.g. to print warnings about them), but Aleksa asked for it [1]. I haven't used it in runtimetest. I'm waiting for the in-flight 7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors, 2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the runtimetest error-processing code with: validations := defaultValidations if platform == "linux" { validations = append(validations, linuxValidations...) } and a single loop over the constructed validations array. I initially had FilterError returning (filtered, removed), but golint didn't like that: error should be the last type when returning multiple items [1]: opencontainers#492 (comment) Signed-off-by: W. Trevor King <[email protected]>
wking
force-pushed
the
validate-compliance-level
branch
from
be12cce to
1cbca20
Compare
This doesn't save much time for folks who are iterating over the removed errors (e.g. to print warnings about them), but Aleksa asked for it [1]. I haven't used it in runtimetest. I'm waiting for the in-flight 7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors, 2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the runtimetest error-processing code with: validations := defaultValidations if platform == "linux" { validations = append(validations, linuxValidations...) } and a single loop over the constructed validations array. I initially had FilterError returning (filtered, removed), but golint didn't like that: error should be the last type when returning multiple items [1]: opencontainers#492 (comment) Signed-off-by: W. Trevor King <[email protected]>
|
be12cce has an API that passes |
cmd/oci-runtime-tool/validate.go
Outdated
| @@ -27,8 +36,13 @@ var bundleValidateCommand = cli.Command{ | |||
| } | |||
|
|
|||
| if err := v.CheckAll(); err != nil { | |||
| removed, err := specerror.FilterError(err, complianceLevel) | |||
There was a problem hiding this comment.
I think removed is not so easy to understand, how about filtered?
There was a problem hiding this comment.
I think
removedis not so easy to understand, how aboutfiltered?
I'm open to rewording, but FilterError is currently using removed for “these were removed from the input error due to their low level” and filtered for “the high-level and non-specerror stuff you still need to worry about”. If we decide to use filtered for the low-level stuff, what should we use for the high-level and non-specerror stuff?
There was a problem hiding this comment.
How about just printing removed errors in FilterError, FilterError just returns filtered errors?
There was a problem hiding this comment.
How about just printing
removed errorsin FilterError, FilterError just returns filtered errors?
I'd rather not make UI decisions in a library function. Maybe the caller wants to silently ignore low-level errors. Or maybe they want to present them as TAP diagnostics (#439). Returning the low-level error slice leaves that sort of thing up to the caller.
There was a problem hiding this comment.
but removed, err := specerror.FilterError(err, complianceLevel) looks very strange. That looks like removed is the expected result, err represents error message.
There was a problem hiding this comment.
but
removed, err := specerror.FilterError(err, complianceLevel)looks very strange. That looks like removed is the expected result, err represents error message.
So maybe low, high, alwaysNil := specerror.FilterError(...)?
There was a problem hiding this comment.
I think alwaysNil is also not so good. How about just returning a struct includes filtered and low errors?
There was a problem hiding this comment.
I think
alwaysNilis also not so good. How about just returning a struct includesfilteredandlowerrors?
Works for me; I'll reroll in a few hours.
wking
force-pushed
the
validate-compliance-level
branch
from
1cbca20 to
6089f63
Compare
This doesn't save much time for folks who are iterating over the removed errors (e.g. to print warnings about them), but Aleksa asked for it [1] and Ma suggested the helper struct [2]. I haven't used it in runtimetest. I'm waiting for the in-flight 7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors, 2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the runtimetest error-processing code with: validations := defaultValidations if platform == "linux" { validations = append(validations, linuxValidations...) } and a single loop over the constructed validations array. [1]: opencontainers#492 (comment) [2]: opencontainers#492 (comment) Signed-off-by: W. Trevor King <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
The man-page entry and Bash completions for this option were added in 4029999 (#354), but the option was left unimplemented. The implementation in this commit is very similar to the existing runtimetest implementation from 4029999, except we warn instead of silently skipping non-fatal spec violations (more on that here).
The warning (vs. error) on invalid level arguments follows the runtimetest implementation from 6316a4e (#354). I'd personally prefer hard errors on invalid level arguments, but didn't want to break runtime-tools consistency over that.