-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
cmd/loki: minor consistency patch #6364
Conversation
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
- distributor -0.3%
+ querier 0%
+ querier/queryrange 0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @michaelgrigoryan25, but I'm hesitant to merge this because it might have side-effects that are not immediately obvious. To be honest I don't see the value this PR brings besides for slightly more idiomatic Go in this one file.
So maybe you can help me understand by elaborating on what the problem was that you encountered and why you submitted the PR?
Hey @dannykopping, thanks for reviewing my PR!
It makes the code just a bit more idiomatic, and easier to read. Besides that, nothing has changed functionally.
What side effects might it have? I don't think that it would have any, since all the errors are initialized separately (except the last one), and are immediately returned after being checked. Kindly correct me if I'm wrong though :) Edit: Also, in the latest commit, I fixed a typo in the word |
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
@michaelgrigoryan25 OK, thank you. Looking at it more closely I can't see any issues. |
Thank you @dannykopping. I have successfully signed the CLA. |
What this PR does / why we need it:
Consider this PR as a refactor.
Checklist
CHANGELOG.md
.docs/sources/upgrading/_index.md