Skip to content
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

Add -log-level flag to controller #348

Merged
merged 3 commits into from
Oct 7, 2020
Merged

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Oct 2, 2020

No description provided.

@lkysow lkysow requested review from a team, ishustava and thisisnotashwin and removed request for a team October 2, 2020 20:57
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦊

@@ -231,6 +255,24 @@ func (c *Command) Run(args []string) int {
return 0
}

// toLevel returns the zapcore level and an error if lvl is not supported.
func toLevel(lvl string) (zapcore.Level, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use the UnmarshalText function from the zapcore library. Not sure if you saw it and decided not to use it for some reason. The only difference between theirs and your implementation is that theirs also accepts an all-caps string for log level, but I don't think it would be bad if we do too. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also switched to using their constants. The one maybe issue is that they actually support more levels than we've documented but... probably okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Yeah, I think that's fine, given that we might switch it out to hclog.

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@lkysow lkysow merged commit e9cc25b into crd-controller-base Oct 7, 2020
@lkysow lkysow deleted the crd-logging branch October 7, 2020 19:03
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants