pkg/cdi: add missing error checks in cache #183
Conversation
Mostly retain the existing behavior of ignoring errors but using '_' to make golangci-lint happy. Signed-off-by: Markus Lehtonen <[email protected]>
|
I was not completely confident I'm doing the right thing here. Thus draft :) As a general note I think the very common pattern here of omitting error checking is bad design. Either check it or then don't even return an error in the first place. |
|
Syntactically, I think this is fine. I'll defer to @klihub for when we should actually return the errors when refreshing the cache. |
| @@ -69,7 +69,9 @@ func NewCache(options ...Option) (*Cache, error) { | |||
| watch: &watch{}, | |||
| } | |||
|
|
|||
| WithSpecDirs(DefaultSpecDirs...)(c) | |||
| if err := WithSpecDirs(DefaultSpecDirs...)(c); err != nil { | |||
There was a problem hiding this comment.
WithSpecDirs is an Option which can't fail, so this is definitely a behavior-unaltering change and as such definitely fine.
There was a problem hiding this comment.
Hmm, ok, this lead me to submit #187 (replaces this particular change)
| c.refresh() | ||
|
|
||
| return nil | ||
| return c.refresh() |
There was a problem hiding this comment.
TBH, off the top of my head I'm not sure if this could cause problems.
Looking at the code, maybe it would be better/simpler/cleaner to change the API so that [Rr]efresh() never returns an error. Since errors encountered during the last refresh can anyway be queried using GetErrors(), if a caller is interested in errors it can query them itself.
|
@marquiz this can be closed now, correct? |
|
We agreed to not break the Cache API (return an error from |
Mostly retain the existing behavior of ignoring errors but using
_to make golangci-lint happy.