Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Add Logger and support log level #437

Open
noaabarki opened this issue Feb 24, 2022 · 13 comments
Open

Add Logger and support log level #437

noaabarki opened this issue Feb 24, 2022 · 13 comments
Assignees
Labels
enhancement New feature or request up for grabs Looking for a contributor to take this task

Comments

@noaabarki
Copy link
Contributor

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 noaabarki added enhancement New feature or request up for grabs Looking for a contributor to take this task labels Feb 24, 2022
@imrushi
Copy link
Contributor

imrushi commented Mar 10, 2022

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?

@TzlilSwimmer123
Copy link
Contributor

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

@noaabarki
Copy link
Contributor Author

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. 💪🏻😎

@imrushi
Copy link
Contributor

imrushi commented Apr 12, 2022

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.

@noaabarki
Copy link
Contributor Author

noaabarki commented Apr 13, 2022

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

@amustaque97
Copy link
Contributor

amustaque97 commented Nov 2, 2022

Hey, I want to work on this issue.

@imrushi
Copy link
Contributor

imrushi commented Nov 3, 2022

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

@imrushi imrushi removed their assignment Nov 3, 2022
@adifayer
Copy link
Contributor

adifayer commented Nov 3, 2022

@amustaque97 You got it 🤗

@amustaque97
Copy link
Contributor

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

@myishay
Copy link
Contributor

myishay commented Nov 6, 2022

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?

@amustaque97
Copy link
Contributor

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.

@noaabarki
Copy link
Contributor Author

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

@myishay
Copy link
Contributor

myishay commented Nov 20, 2022

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request up for grabs Looking for a contributor to take this task
Projects
None yet
Development

No branches or pull requests

6 participants