-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Updated Meshkit logger docs #11202
base: master
Are you sure you want to change the base?
Updated Meshkit logger docs #11202
Conversation
Signed-off-by: Aviral0702 <aviral.asthana0704@gmail.com>
🚀 Preview for commit 8f4146c at: https://667086e9c8500f1f446bb42e--meshery-docs-preview.netlify.app |
@@ -145,7 +145,7 @@ func main() { | |||
DebugLevel: true, | |||
}) | |||
if err != nil { | |||
fmt.Println(err) | |||
logrus.Error(err) |
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.
We're looking for MeshKit logger, not logrus, right?
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.
Sorry @leecalcote for that. I think I misunderstood the task.
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.
@singh1203 will you offer some direction?
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.
Yes, @Aviral0702 we are in motion to get rid of logrus with Meshkit logger.
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.
You might give it a read to starting para's of this particular doc itself.
@@ -145,7 +145,7 @@ func main() { | |||
DebugLevel: true, | |||
}) | |||
if err != nil { | |||
fmt.Println(err) | |||
logrus.Error(err) |
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.
Yes, @Aviral0702 we are in motion to get rid of logrus with Meshkit logger.
@@ -145,7 +145,7 @@ func main() { | |||
DebugLevel: true, |
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.
well issue also points to hint in example for LogLevel
.
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.
Should I use LogLevel instead of DebugLevel there ?
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.
Line 77 in 6889021
LogLevel: logLevel, |
Signed-off-by: Aviral0702 <aviral.asthana0704@gmail.com>
🚀 Preview for commit fe98fb3 at: https://66715331638814eced6bc8d3--meshery-docs-preview.netlify.app |
@@ -142,10 +142,10 @@ var ( | |||
func main() { | |||
log, err := logger.New("test", logger.Options{ | |||
Format: logger.SyslogLogFormat, | |||
DebugLevel: true, | |||
LogLevel: logLevel, |
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.
maintain the indentation
Signed-off-by: Aviral0702 <aviral.asthana0704@gmail.com>
🚀 Preview for commit c01f68b at: https://66716b26ff903df44f9ec562--meshery-docs-preview.netlify.app |
Signed-off-by: Aviral0702 <aviral.asthana0704@gmail.com>
🚀 Preview for commit b352a6c at: https://66716def6fd90b03fda4708e--meshery-docs-preview.netlify.app |
🚀 Preview for commit 1ff6675 at: https://667205d36a08c662cfeb4998--meshery-docs-preview.netlify.app |
@@ -142,7 +142,7 @@ var ( | |||
func main() { | |||
log, err := logger.New("test", logger.Options{ | |||
Format: logger.SyslogLogFormat, | |||
DebugLevel: true, | |||
LogLevel: logLevel, |
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.
@Aviral0702 Good job utilizing the loglevel here, but do you know what it is? Is it a variable or a string?
If it is a variable, does it have a value assigned to it?
Answer: Refer to the example provided under the issue.
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.
Yes, It is an integer. First of all it is assigned with the value of LOG_LEVEL and there is a check for DEBUG_LEVEL which returns a bool value if it comes out to be true then then the value of logLevel is assigned with the value of debugLevel.
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.
Done with the help of Viper framework of Golang.
🚀 Preview for commit d341117 at: https://66729f3c9b6d64e79c03660f--meshery-docs-preview.netlify.app |
1ff6675
to
b352a6c
Compare
🚀 Preview for commit b352a6c at: https://6672a578d230f5e76f00e701--meshery-docs-preview.netlify.app |
🚀 Preview for commit 6ebe5b3 at: https://6672af0cedca86f775351c1e--meshery-docs-preview.netlify.app |
@@ -142,7 +142,7 @@ var ( | |||
func main() { | |||
log, err := logger.New("test", logger.Options{ |
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.
@Aviral0702 You can take a look to this example set the logLevel
from here
Signed-off-by: Aviral0702 <aviral.asthana0704@gmail.com>
🚀 Preview for commit 8157ff7 at: https://667513368132602796471328--meshery-docs-preview.netlify.app |
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.
LGTM!
🚀 Preview for commit c827505 at: https://66752cdca47af8371fd37110--meshery-docs-preview.netlify.app |
@@ -140,9 +142,13 @@ var ( | |||
) |
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.
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 the review. I am trying to implement the changes as you said.
🚀 Preview for commit b2fabaf at: https://6675d440edf85be196ad5b6d--meshery-docs-preview.netlify.app |
Signed-off-by: Aviral0702 aviral.asthana0704@gmail.com
Notes for Reviewers
This PR fixes #11194
Current Behavior:
Signed commits