✨ Add helpers to configure logger options via pflags #767

bharathi-tenneti · 2020-01-20T19:07:47Z

This PR allows logging flags(/pkg/log/zap), to be passed from command line in addition to functional calls.
Follow up from #639

k8s-ci-robot · 2020-01-20T19:07:56Z

Welcome @bharathi-tenneti!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-01-20T19:07:57Z

Hi @bharathi-tenneti. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/log/zap/zap.go

hasbro17

Some suggestions for cleaning up the var names a bit.

pkg/log/zap/flags.go

pkg/log/zap/zap.go

joelanford

Somewhere in here, we probably need to make sure that sampling is turned off when we're logging at a debug level (if we aren't already), because it will cause a panic.

See: https://github.com/operator-framework/operator-sdk/blob/633b92c888ad3eeb9704441b7f6d6bd15f933d0e/pkg/log/zap/logger.go#L96-L100

pkg/log/zap/flags.go

🏃 add e2e tests for v2

pkg/log/zap/flags.go

camilamacedo86 · 2020-03-04T20:26:02Z

/ok-to-test

estroz

Some suggestions and nits.

pkg/log/zap/zap.go

pkg/log/zap/flags.go

estroz

/lgtm

hasbro17 · 2020-03-09T06:03:11Z

pkg/log/zap/zap.go

@@ -207,3 +208,44 @@ func NewRaw(opts ...Opts) *zap.Logger {
 	log = log.WithOptions(o.ZapOpts...)
 	return log
 }
+
+// BindToFlagSet func allows CLI passed flags to be parsed.


Suggested change

// BindToFlagSet func allows CLI passed flags to be parsed.

// BindFlagSet will parse the given flagset for zap option flags and set the log options accordingly

Also for the godoc comment here, can you add the list of flag names that BindFlags currently parses.

pkg/log/zap/zap.go

Co-Authored-By: Haseeb Tariq <hasbro17@gmail.com>

shawn-hurley · 2020-03-11T19:28:50Z

/approve

k8s-ci-robot · 2020-03-11T19:29:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bharathi-tenneti, shawn-hurley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [shawn-hurley]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gerred · 2020-03-11T19:38:05Z

/lgtm

hasbro17 · 2020-03-11T20:33:50Z

@gerred This got merged with the commits unsquashed.
https://github.com/kubernetes-sigs/controller-runtime/commits/master

@DirectXMan12 I don't think we can clean this up now but is there any way to avoid this in the future? e.g have the bot squash them for us or a linter to make sure we clean up before merging.

gerred · 2020-03-12T02:50:42Z

@hasbro17 Yeah, this ended up being a little lax.

@shawn-hurley maybe we reverse the relationship in the future as well. As an approver I'm typically looking for those squashed commits when I approve, but I didn't take the due diligence to look back and see that it had already been approved before I /lgtmed. I'll do so in the future, but we should work together to make sure the reviewer is looking at the changes in and of itself and the approver is doing the same work, but also making sure it meets the criteria for being merged. I think it'd nbd but we can always stand to improve project hygiene!

DirectXMan12 · 2020-03-18T21:25:36Z

oh yeah. We should add a CI check or something for this

PR kubernetes-sigs#767 introduces some weird merge commit styles into the git history by default, so now we have to tolerate them in our release notes script.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 20, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 20, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2020

k8s-ci-robot requested review from DirectXMan12 and pwittrock January 20, 2020 19:08

bharathi-tenneti requested review from hasbro17 and joelanford and removed request for DirectXMan12 and pwittrock January 20, 2020 19:08

DirectXMan12 reviewed Jan 22, 2020

View reviewed changes

pkg/log/zap/zap.go Outdated Show resolved Hide resolved

hasbro17 suggested changes Jan 23, 2020

View reviewed changes

hasbro17 reviewed Jan 23, 2020

View reviewed changes

pkg/log/zap/zap.go Outdated Show resolved Hide resolved

joelanford requested changes Jan 23, 2020

View reviewed changes

pkg/log/zap/flags.go Outdated Show resolved Hide resolved

pkg/log/zap/flags.go Outdated Show resolved Hide resolved

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #767 from mengqiy/travise2e

de15359

🏃 add e2e tests for v2

joelanford reviewed Feb 4, 2020

View reviewed changes

pkg/log/zap/flags.go Outdated Show resolved Hide resolved

bharathi-tenneti changed the title ~~[WIP] Merge logging flags into the controller-runtime from operator-SDK~~ [WIP] Add helpers to configure logger options via pflags Feb 4, 2020

bharathi-tenneti changed the title ~~[WIP] Add helpers to configure logger options via pflags~~ [WIP]:sparkles: Add helpers to configure logger options via pflags Feb 4, 2020

bharathi-tenneti changed the title ~~[WIP]:sparkles: Add helpers to configure logger options via pflags~~ [WIP] [:sparkles:] Add helpers to configure logger options via pflags Feb 4, 2020

bharathi-tenneti changed the title ~~[WIP] [:sparkles:] Add helpers to configure logger options via pflags~~ [WIP] ✨ Add helpers to configure logger options via pflags Feb 4, 2020

bharathi-tenneti changed the title ~~[WIP] ✨ Add helpers to configure logger options via pflags~~ [WIP] ✨ Add helpers to configure logger options via pflags Feb 4, 2020

hasbro17 reviewed Feb 4, 2020

View reviewed changes

pkg/log/zap/flags.go Outdated Show resolved Hide resolved

DirectXMan12 assigned hasbro17 Feb 5, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 19, 2020

bharathi-tenneti added 2 commits February 20, 2020 15:40

Flags supported added

9ed6d0a

stracktrace flag added - not working

c9a71ba

camilamacedo86 reviewed Mar 4, 2020

View reviewed changes

pkg/log/zap/flags.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2020

estroz suggested changes Mar 4, 2020

View reviewed changes

pkg/log/zap/zap.go Outdated Show resolved Hide resolved

pkg/log/zap/zap.go Outdated Show resolved Hide resolved

pkg/log/zap/zap.go Outdated Show resolved Hide resolved

pkg/log/zap/flags.go Outdated Show resolved Hide resolved

bharathi-tenneti added 2 commits March 5, 2020 11:10

map added for levelStrings replacing switch, and formatting changes

dd4c92b

Fixing few Error messaages

d17d9c9

estroz approved these changes Mar 5, 2020

View reviewed changes

k8s-ci-robot assigned estroz Mar 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2020

hasbro17 suggested changes Mar 9, 2020

View reviewed changes

Update pkg/log/zap/zap.go

e7d7f4c

Co-Authored-By: Haseeb Tariq <hasbro17@gmail.com>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2020

bharathi-tenneti added 3 commits March 10, 2020 11:17

Header comments fixed for godoc

a1ebd06

Merge branch 'master' of github.com:bharathi-tenneti/controller-runtime

06787b6

Merge branch 'master' of github.com:kubernetes-sigs/controller-runtime

4ced175

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2020

k8s-ci-robot assigned gerred Mar 11, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2020

k8s-ci-robot merged commit 5aca8b7 into kubernetes-sigs:master Mar 11, 2020

DirectXMan12 mentioned this pull request Mar 18, 2020

🏃 Tolerate (more) merge commits in release notes #864

Merged

bharathi-tenneti mentioned this pull request Apr 20, 2020

Add log option and flag for zap time format #912

Closed

StevenACoffman mentioned this pull request Oct 9, 2020

OSDK->KB: Logging Flags kubernetes-sigs/kubebuilder#885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add helpers to configure logger options via pflags #767

✨ Add helpers to configure logger options via pflags #767

bharathi-tenneti commented Jan 20, 2020

k8s-ci-robot commented Jan 20, 2020

k8s-ci-robot commented Jan 20, 2020

hasbro17 left a comment

joelanford left a comment

camilamacedo86 commented Mar 4, 2020

estroz left a comment

estroz left a comment

hasbro17 Mar 9, 2020

shawn-hurley commented Mar 11, 2020

k8s-ci-robot commented Mar 11, 2020

gerred commented Mar 11, 2020

hasbro17 commented Mar 11, 2020

gerred commented Mar 12, 2020

DirectXMan12 commented Mar 18, 2020

	// BindToFlagSet func allows CLI passed flags to be parsed.
	// BindFlagSet will parse the given flagset for zap option flags and set the log options accordingly

✨ Add helpers to configure logger options via pflags #767

✨ Add helpers to configure logger options via pflags #767

Conversation

bharathi-tenneti commented Jan 20, 2020

k8s-ci-robot commented Jan 20, 2020

k8s-ci-robot commented Jan 20, 2020

hasbro17 left a comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Mar 4, 2020

estroz left a comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

hasbro17 Mar 9, 2020

Choose a reason for hiding this comment

shawn-hurley commented Mar 11, 2020

k8s-ci-robot commented Mar 11, 2020

gerred commented Mar 11, 2020

hasbro17 commented Mar 11, 2020

gerred commented Mar 12, 2020

DirectXMan12 commented Mar 18, 2020