Clean up logging usage (and minor script improvements) #86

DirectXMan12 · 2018-07-25T20:43:56Z

This fixes up a bunch of places where we were using the go log package in the standard library, or glog, fixes a bug found in the delegating logger, and introduces proper tests for the delegating logger to prevent that from happening again.

It also contains a couple minor quality-of-life improvements to the tests scripts -- you can now run linters and testing separately, and can simply specify KUBEBUILDER_ASSETS instead of exporting three different variables if your kubebuilder lives elsewhere.

The example sets up logr, and then uses the built-in go `log` package. We should show people how to make use of logr instead.

The former abbreviation (`mrg`) is a well known typo. This fixes instances of `mrg` so they say `mgr` (ManaGeR).

We should be consistenly using logr instead of the go standard library, or glog (when in our code -- we don't have any control over code from kubernetes core). Most of this usage was `log.Fatal`, which we replace with calls to `<log>.Error` and `os.Exit(1)`.

Add a single environment variable that can be set to change the default search path for kubebuilder test assets, `KUBEBUILDER_ASSETS`. This prevents us from having to specify all three `TEST_ASSET_<thing>` environment variables manually if kubebuilder isn't installed in /usr/local/kubebuilder.

droot · 2018-07-26T18:38:37Z

Thanks for the PR @DirectXMan12 . Took a quick look and looks good.

@mengqiy Can you also pl. take a look at the changes in the admission pkg ?

mengqiy

/lgtm

mengqiy · 2018-07-30T21:30:37Z

pkg/admission/cert/writer/fs.go

@@ -148,7 +147,7 @@ func (f *fsCertWriter) doWrite(webhookName string) (*certgenerator.Artifacts, er
 	if err != nil {
 		return nil, err
 	}
-	aw, err := atomic.NewAtomicWriter(v.path, fmt.Sprintf("processing webhook %q", webhookName))
+	aw, err := atomic.NewAtomicWriter(v.path, log.WithName("atomic-writer").WithValues("task", "processing webhook", "webhook", webhookName))


Not in the scope of this PR.
I feel the signature of method WithValues is a little confusing, if I don't know that the list of interface with 2*n items is actually representing n key-value pairs.
Why not using a signature that taking a map?

the interface is mostly copied from Zap, so using a map means more overhead in our primary usecase. Beyond that, Zap specifically does it to allow certain tricks that we don't take advantage of now, but we could consider in the future (i.e. helpers which have their own tag built in, improvements around number of allocations, etc). Plus, it's fewer keystrokes ;-). I do understand where you're coming from, though.

mengqiy · 2018-07-30T21:36:43Z

pkg/runtime/log/log.go

 )

 // ZapLogger is a Logger implementation.
-// if development is true stack traces will be printed for errors
+// If development is true, a Zap development config ill be used


mengqiy · 2018-07-30T21:41:14Z

pkg/admission/cert/writer/internal/atomic/atomic_writer.go

-	targetDir  string
-	logContext string
+	targetDir string
+	log       logr.Logger


Changing this file means that it starts to diverge from upstream.
But I guess it's not a problem.

if we actually have our own copy, we should treat it as such, IMO.

mengqiy · 2018-07-31T19:20:45Z

@DirectXMan12 There is one typo left to fix. Then it's good to go.

This adds a helper (ZapLoggerTo) to log to a particular io.Writer. This allows us to set our suite logging to `GinkgoWriter`, so that ginkgo captures the logging output correctly.

This fixes the tag handling of the delegating logger, which was missing a splat operator, and thus causing the tags to be misinterpretted. This also adds proper testing of the delegating functionality, so that this doesn't happen again.

This splits up test.sh so that the individual parts can be run separately (particularly, the linting, the testing and coverage, and the downloading of tools, etc). All the scripts now live in the hack directory.

DirectXMan12 · 2018-08-02T16:13:05Z

@mengqiy fixes should be in place now, so this should be ready to merge.

mengqiy · 2018-08-02T17:35:12Z

/lgtm

k8s-ci-robot · 2018-08-02T17:36:40Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: DirectXMan12

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

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

…std-log Clean up logging usage (and minor script improvements)

Embbed more example types for docs

DirectXMan12 added 2 commits July 25, 2018 15:49

Fix up example to make use of logr

ecee73e

The example sets up logr, and then uses the built-in go `log` package. We should show people how to make use of logr instead.

Fix the abbreviation for manager

f58406d

The former abbreviation (`mrg`) is a well known typo. This fixes instances of `mrg` so they say `mgr` (ManaGeR).

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 25, 2018

DirectXMan12 added 2 commits July 25, 2018 16:44

DirectXMan12 force-pushed the bug/dont-use-std-log branch 2 times, most recently from 5390586 to 88fd91b Compare July 26, 2018 00:36

DirectXMan12 mentioned this pull request Jul 26, 2018

Can't run tests locally #90

Closed

DirectXMan12 requested a review from mengqiy July 27, 2018 18:28

k8s-ci-robot assigned mengqiy Jul 30, 2018

mengqiy approved these changes Jul 30, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2018

DirectXMan12 added 3 commits July 31, 2018 16:17

Add helper to log to a particular writer

19b9f17

This adds a helper (ZapLoggerTo) to log to a particular io.Writer. This allows us to set our suite logging to `GinkgoWriter`, so that ginkgo captures the logging output correctly.

Fix delegating logger tag handling and test

34518d5

This fixes the tag handling of the delegating logger, which was missing a splat operator, and thus causing the tags to be misinterpretted. This also adds proper testing of the delegating functionality, so that this doesn't happen again.

Split up test.sh

d3231aa

This splits up test.sh so that the individual parts can be run separately (particularly, the linting, the testing and coverage, and the downloading of tools, etc). All the scripts now live in the hack directory.

DirectXMan12 force-pushed the bug/dont-use-std-log branch from 88fd91b to d3231aa Compare July 31, 2018 20:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2018

mengqiy added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2018

k8s-ci-robot merged commit d12eff3 into kubernetes-sigs:master Aug 2, 2018

DirectXMan12 deleted the bug/dont-use-std-log branch August 3, 2018 15:47

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#86 from DirectXMan12/bug/dont-use-…

b80666f

…std-log Clean up logging usage (and minor script improvements)

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

Merge pull request #86 from apelisse/master

7c4b96a

Embbed more example types for docs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clean up logging usage (and minor script improvements) #86

Clean up logging usage (and minor script improvements) #86

Uh oh!

DirectXMan12 commented Jul 25, 2018

Uh oh!

droot commented Jul 26, 2018

Uh oh!

mengqiy left a comment

Uh oh!

mengqiy Jul 30, 2018

Uh oh!

DirectXMan12 Jul 31, 2018

Uh oh!

mengqiy Jul 30, 2018

Uh oh!

mengqiy Jul 30, 2018

Uh oh!

DirectXMan12 Jul 31, 2018

Uh oh!

mengqiy commented Jul 31, 2018

Uh oh!

DirectXMan12 commented Aug 2, 2018

Uh oh!

mengqiy commented Aug 2, 2018

Uh oh!

k8s-ci-robot commented Aug 2, 2018

Uh oh!

Uh oh!

Clean up logging usage (and minor script improvements) #86

Clean up logging usage (and minor script improvements) #86

Uh oh!

Conversation

DirectXMan12 commented Jul 25, 2018

Uh oh!

droot commented Jul 26, 2018

Uh oh!

mengqiy left a comment

Choose a reason for hiding this comment

Uh oh!

mengqiy Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

mengqiy Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

mengqiy Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Jul 31, 2018

Uh oh!

DirectXMan12 commented Aug 2, 2018

Uh oh!

mengqiy commented Aug 2, 2018

Uh oh!

k8s-ci-robot commented Aug 2, 2018

Uh oh!

Uh oh!