Add basic validation of configs #763
Conversation
Codecov Report
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
+ Coverage 42.59% 43.43% +0.84%
==========================================
Files 70 70
Lines 4029 4089 +60
==========================================
+ Hits 1716 1776 +60
Misses 2208 2208
Partials 105 105
Continue to review full report at Codecov.
|
zubron
force-pushed
the
default-configs-394
branch
from
4461e74
to
37a38f6
Compare
pkg/client/interfaces.go
Outdated
| @@ -29,6 +29,11 @@ import ( | |||
| "k8s.io/client-go/rest" | |||
| ) | |||
|
|
|||
| // ConfigValidator allows the command configurations to be validated. | |||
| type ConfigValidator interface { | |||
There was a problem hiding this comment.
nit: I'd name this just Validator since it is just a one function interface with that function. Nothing about the interface is config-specific.
There was a problem hiding this comment.
In addition, I would actually just call this validator. Users of the library:
- probably dont need the validator type at all, that is just for our own testing
- if they do need it, they can define their own validator interface in 2-3 lines.
Keeping this unexported helps limit dependencies we feel obligated to support. We can always export it later if we come up with some important reason to do so.
There was a problem hiding this comment.
Ah, I didn't think about the impact of exporting it. I will change that.
pkg/client/interfaces.go
Outdated
| return errors.Wrap(err, "GenConfig validation failed") | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
On all of these Validate methods you can actually make them one line and just do:
return errors.Wrap(err, "GenConfig validation failed")
The reason be that errors.Wrap will return nil if the error is nil so that you don't always have to do this if err := ... block.
There was a problem hiding this comment.
Nice! I didn't realise that. Will update these.
pkg/client/retrieve.go
Outdated
| ec := make(chan error, 1) | ||
| client, err := c.Client() | ||
| if err != nil { | ||
| ec <- err | ||
| return nil, ec | ||
| return nil, nil, err |
There was a problem hiding this comment.
In this case there isn't a need to put the err on the channel (line above this) since we aren't even returning it.
There was a problem hiding this comment.
Good spot. I missed that!
pkg/client/defaults.go
Outdated
| @@ -51,7 +52,7 @@ func NewDeleteConfig() *DeleteConfig { | |||
| // NewLogConfig is a LogConfig with follow disabled and default images. | |||
| func NewLogConfig() *LogConfig { | |||
| return &LogConfig{ | |||
| Follow: false, | |||
| Namespace: config.DefaultNamespace, | |||
There was a problem hiding this comment.
Since we are leaving in the namespace defaulting in the other methods, we should do the same here, right? Was there some other reason to specifically pull out namespace from this and the delete config?
pkg/client/interfaces.go
Outdated
| // Validate checks the config to determine if it is valid. | ||
| func (lc *LogConfig) Validate() error { | ||
| if lc.Namespace == "" { | ||
| return errors.New("Namespace cannot be empty") |
There was a problem hiding this comment.
nit: errors should be lower cased rather than using sentence capitalization. This applies throughout.
See: https://github.com/golang/go/wiki/CodeReviewComments#error-strings
zubron
force-pushed
the
default-configs-394
branch
5 times, most recently
from
84d9500
to
07d599b
Compare
This adds basic checking and validation of configs passed to some of the interface function implementations in the Sonobuoy client. It is primarily being added to prevent panics caused by passing nil configs to these functions. It could be expanded in future to provide more comprehensive validation of configuration objects. Fixes vmware-tanzu#394 Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
zubron
force-pushed
the
default-configs-394
branch
from
07d599b
to
0fb7192
Compare
What this PR does / why we need it:
This adds basic checking and validation of configs passed to some of the
interface function implementations in the Sonobuoy client. It is primarily
being added to prevent panics caused by passing nil configs to these
functions. It could be expanded in future to provide more comprehensive
validation of configuration objects.
Fixes #394
Signed-off-by: Bridget McErlean bmcerlean@vmware.com
Which issue(s) this PR fixes
Special notes for your reviewer:
This does not include comprehensive validation for some of the more
complex configuration objects (e.g.
GenConfig). It primarily focuses onthe nil checks. A lot of the added tests in the client don't test the
"successful" case, where the execution of the function would proceed
past the validation. This is due to the complexity of the test set up that
would be required which seemed out of scope for this change.
Release note: