Skip to content
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

Enable errcheck linter #4462

Merged
merged 4 commits into from
Nov 23, 2021
Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Nov 19, 2021

Description:

Enable errcheck and fix linter errors.

Link to tracking Issue: Updates #2789

@mx-psi mx-psi requested review from a team and bogdandrutu November 19, 2021 13:17
@codecov
Copy link

codecov bot commented Nov 19, 2021

Codecov Report

Merging #4462 (89e3a44) into main (bdcb989) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4462   +/-   ##
=======================================
  Coverage   90.70%   90.71%           
=======================================
  Files         178      178           
  Lines       10357    10367   +10     
=======================================
+ Hits         9394     9404   +10     
  Misses        745      745           
  Partials      218      218           
Impacted Files Coverage Δ
config/configmap.go 82.14% <100.00%> (+0.16%) ⬆️
obsreport/obsreport_processor.go 94.69% <100.00%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdcb989...89e3a44. Read the comment docs.

Comment on lines 119 to 120
// Ignore error for now; it only fails with strict merge when the types are different
_ = l.k.Merge(merged)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should Koanf decide to return an error for a different reason we would have no way to report the Set failed, so maybe we should return the error here instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems there's at least a couple of calls that could trigger an error in this func with Load just above this line, I would be in favour of returning an error from Set

Copy link
Member Author

@mx-psi mx-psi Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4467 for this, I want to keep this PR for non-breaking changes

@@ -138,7 +137,7 @@ issues:
# The list of ids of default excludes to include or disable. By default it's empty.
# See the list of default excludes here https://golangci-lint.run/usage/configuration.
include:
- EXC0001
# - EXC0001 - errcheck checks that are not usually checked
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this on #2881 (comment)

@codeboten codeboten added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 19, 2021
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, it looks good to me. Will review and approve once a decision on what to do w/ the errors from Set is made.

Comment on lines 119 to 120
// Ignore error for now; it only fails with strict merge when the types are different
_ = l.k.Merge(merged)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems there's at least a couple of calls that could trigger an error in this func with Load just above this line, I would be in favour of returning an error from Set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants