datree icon indicating copy to clipboard operation
datree copied to clipboard

Add Logger and support log level

Open noaabarki opened this issue 2 years ago β€’ 13 comments

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

  1. Use logrus package. [Read more](https://github.com/sirupsen/logrus)
  2. Switch errors with logrus.Error.

noaabarki avatar Feb 24 '22 14:02 noaabarki

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?

imrushi avatar Mar 10 '22 05:03 imrushi

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 :)

TzlilSwimmer123 avatar Mar 15 '22 11:03 TzlilSwimmer123

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. πŸ’ͺ🏻😎

noaabarki avatar Apr 12 '22 07:04 noaabarki

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 avatar Apr 12 '22 07:04 imrushi

@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.

noaabarki avatar Apr 13 '22 11:04 noaabarki

Hey, I want to work on this issue.

amustaque97 avatar Nov 02 '22 14:11 amustaque97

Hi @amustaque97, You can take this issue ahead. Sorry for the inconvenience.

imrushi avatar Nov 03 '22 05:11 imrushi

@amustaque97 You got it πŸ€—

adifayer avatar Nov 03 '22 09:11 adifayer

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

Looking forward to hearing from you @adifayer

amustaque97 avatar Nov 04 '22 07:11 amustaque97

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?

myishay avatar Nov 06 '22 17:11 myishay

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.

amustaque97 avatar Nov 07 '22 10:11 amustaque97

@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 avatar Nov 20 '22 08:11 noaabarki

@noaabarki hmm I understand. @amustaque97 I think you can go ahead and start implementation then! no change of scope

myishay avatar Nov 20 '22 13:11 myishay