*: Added errcheck and fixed not check errors [PART1]. #389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
domgreen
reviewed
Jun 27, 2018
cmd/thanos/sidecar_test.go
Outdated
| @@ -23,12 +24,12 @@ global: | |||
| testutil.Ok(t, err) | |||
|
|
|||
| testutil.Ok(t, p.Start()) | |||
| defer p.Stop() | |||
| defer func() { _ = p.Stop() }() | |||
There was a problem hiding this comment.
out of interest why this change? Is it due to error check needing to assign the error?
There was a problem hiding this comment.
yes, errcheck requires explicit return to make sure we don't forget about someting. I think it makes sense. Let's use testutil.Ok here actually maybe (:
domgreen
reviewed
Jun 27, 2018
pkg/runutil/runutil.go
Outdated
| } | ||
|
|
||
| // There is already error, let's log this one. | ||
|
|
domgreen
reviewed
Jun 27, 2018
| @@ -363,33 +364,33 @@ func Repair(dir string, id ulid.ULID, source SourceType, ignoreChkFns ...ignoreF | |||
| if err != nil { | |||
| return resid, errors.Wrap(err, "open block") | |||
| } | |||
| defer b.Close() | |||
| defer runutil.BestEffortErr(nil, &err, b, "repair block reader") | |||
There was a problem hiding this comment.
how do you choose between BestEffortErr and LogOnErr?
There was a problem hiding this comment.
Ignore - I read the code again 😄
domgreen
reviewed
Jun 27, 2018
pkg/runutil/runutil.go
Outdated
| logger = log.NewLogfmtLogger(os.Stderr) | ||
| } | ||
|
|
||
| level.Warn(logger).Log("msg", "detected best effort error", "err", errors.Wrap(err, fmt.Sprintf(wrap, a...))) |
There was a problem hiding this comment.
This msg makes me think of the BestEffortErr func rather than LogOnErr ... maybe LogOnErr error closing?
domgreen
approved these changes
Jun 27, 2018
Due to high number of these issues, let's split the PR. Signed-off-by: Bartek Plotka <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Due to a high number of these issues, let's split the PR.
Thanks @asbjxrn for finding some not checked errors. This motivates us to fix all of these places. Some of them are serious, some not, but I think it is better to check all errors 100% time. (:
Signed-off-by: Bartek Plotka [email protected]
Changes
runutilhelpers for closing functions.Verification
CI, errcheck