-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
*: Fix issue #7550 - Check correct error when opening rule files in tools #7552
*: Fix issue #7550 - Check correct error when opening rule files in tools #7552
Conversation
Hey, Please review the changes and suggest potential improvements. Thanks! |
e7e6861
to
2f7095b
Compare
Hey @saswatamcode, Could you please review this PR and suggest potential improvements? Thank you! |
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.
Thanks!
But I think PR and issue title are a bit misleading. Maybe should be something like, "Check correct error when opening rule files in tools"?
…s.go Signed-off-by: Nishant Bansal <nishant.bansal.mec21@iitbhu.ac.in>
29854d6
to
05b39c1
Compare
@@ -55,7 +55,6 @@ func checkRulesFiles(logger log.Logger, patterns *[]string) error { | |||
if err != nil || matches == nil { | |||
err = errors.New("matching file not found") | |||
level.Error(logger).Log("result", "FAILED", "error", err) | |||
level.Info(logger).Log() |
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.
Do we need to remove this?
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.
These are just empty logs, Its output is - level=info
, So I removed all level.Info(logger).Log()
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.
I think it might be intended to leave some spaces between lines.
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.
Ok, Should I reintroduce all instances of level.Info(logger).Log()
throughout the tools.go, or only in this specific part(line 58) of the code?
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.
I don't have strong preference. I am ok either way to keep them or remove them.
Hey @saswatamcode, |
…s.go (thanos-io#7552) Signed-off-by: Nishant Bansal <nishant.bansal.mec21@iitbhu.ac.in>
Changes
Fixes #7550
There is a bug in this part of the code. If there is an issue with opening the file through
os.Open()
, we will not be able to obtain exact error logs for debugging. It should return error logs in case there is an issue withos.Open()
.So, I fixed this bug and added test coverage.
Verification
Run the Tests: Execute the following command to run the tests and verify that the bug has been fixed and the new tests are functioning correctly:
Check Test Results: Ensure that all tests pass and that there are no error messages related to the issue with os.Open()