datree
datree copied to clipboard
Add Logger and support log level
Describe the solution you'd like We want to add logger in order to provide more useful info and help our user more conveniently detect errors.
Requirements Golang basic level.
βHow to Implementβ suggestion
- Use
logrus
package. [Read more](https://github.com/sirupsen/logrus) - Switch errors with
logrus.Error
.
Hi @noaabarki , I can work on this issue. ππ¨βπ
In this issue, we just need to add Error logs or others also (like Info, Warning, Fatal...)? And In which Directory cmd
only or any others also please let me know?
Hi @imrushi, thanks for your time!
At the moment we would want a lean version so you can implement the error logs only. The implementation should be in a new folder inside pkg
directory.
Let me know if it's clear now :)
Hi @imrushi! I hope you are well. Just wanted to ping you regarding this issue to check if everything is clear. Do you need any help or guidance? Please, don't hesitate to reach out. πͺπ»π
Hi, @noaabarki I am a little bit confused. in the pkg
folder what should name the folder for error and Error function should take the only the message as a parameter?
We need following function in that package like below format:
func Error(errMsg string) {
logrus.Errorf(errMsg);
}
Let me know if this is right... and correct me if I am wrong.
@imrushi, I'm not sure I understand. The way I see it, we should replace current printings (usingfmt.Print..
) with logrus
and use the correct error level. WDYT? We can discuss more over Slack.
Hey, I want to work on this issue.
Hi @amustaque97, You can take this issue ahead. Sorry for the inconvenience.
@amustaque97 You got it π€
Before jumping into the implementation. Wanted to check there is no change in the scope of the issue. If yes what would it be? If I follow comment, I find only two places where we need to make this change.
![image](https://user-images.githubusercontent.com/21127788/199914883-2b2ac0aa-436f-40ed-ab9d-437460de935b.png)
Looking forward to hearing from you @adifayer
Hey, sorry to interfere in such a late stage of the issue, but I think it might be better using https://github.com/uber-go/zap here since it is already implemented in the https://github.com/datreeio/admission-webhook-datree repo. Maybe even using/modifying the package there to be used in both admission webhook and CLI
Plus, I didn't understand exactly what should be the use of logrus.Error
in the project.
@noaabarki @amustaque97 WDYT?
Plus, I didn't understand exactly what should be the use of logrus.Error in the project.
My understanding is that this might be used in the CI logs.
@myishay I understand that it might be easier for maintenance but still, I don't think it's necessary to have the same logger library in both projects. As @amustaque97 said, since logrus writes log entries as JSON automatically it's more suitable for CI logs.
@noaabarki hmm I understand. @amustaque97 I think you can go ahead and start implementation then! no change of scope