Clean up logging usage (and minor script improvements)

.travis.yml

@@ -18,7 +18,7 @@ install:
 - go get -u gopkg.in/alecthomas/gometalinter.v2 && gometalinter.v2 --install
 
 script:
-- TRACE=1 ./test.sh
+- TRACE=1 ./hack/check-everything.sh
 
 
 # TBD. Suppressing for now.

example/main.go

@@ -19,8 +19,9 @@ package main
 import (
 	"context"
 	"flag"
-	"log"
+	"os"
 
+	"github.com/go-logr/logr"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -36,63 +37,78 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 )
 
+var (
+	log = logf.Log.WithName("example-controller")
+)
+
 func main() {
 	flag.Parse()
 	logf.SetLogger(logf.ZapLogger(false))
+	entryLog := log.WithName("entrypoint")
 
 	// Setup a Manager
-	mrg, err := manager.New(config.GetConfigOrDie(), manager.Options{})
+	mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{})
 	if err != nil {
-		log.Fatal(err)
+		entryLog.Error(err, "unable to set up overall controller manager")
+		os.Exit(1)
 	}
 
 	// Setup a new controller to Reconciler ReplicaSets
-	c, err := controller.New("foo-controller", mrg, controller.Options{
-		Reconciler: &reconcileReplicaSet{client: mrg.GetClient()},
+	c, err := controller.New("foo-controller", mgr, controller.Options{
+		Reconciler: &reconcileReplicaSet{client: mgr.GetClient(), log: log.WithName("reconciler")},
 	})
 	if err != nil {
-		log.Fatal(err)
+		entryLog.Error(err, "unable to set up individual controller")
+		os.Exit(1)
 	}
 
 	// Watch ReplicaSets and enqueue ReplicaSet object key
 	if err := c.Watch(&source.Kind{Type: &appsv1.ReplicaSet{}}, &handler.EnqueueRequestForObject{}); err != nil {
-		log.Fatal(err)
+		entryLog.Error(err, "unable to watch ReplicaSets")
+		os.Exit(1)
 	}
 
 	// Watch Pods and enqueue owning ReplicaSet key
 	if err := c.Watch(&source.Kind{Type: &corev1.Pod{}},
 		&handler.EnqueueRequestForOwner{OwnerType: &appsv1.ReplicaSet{}, IsController: true}); err != nil {
-		log.Fatal(err)
+		entryLog.Error(err, "unable to watch Pods")
+		os.Exit(1)
 	}
 
-	log.Fatal(mrg.Start(signals.SetupSignalHandler()))
+	if err := mgr.Start(signals.SetupSignalHandler()); err != nil {
+		entryLog.Error(err, "unable to run manager")
+		os.Exit(1)
+	}
 }
 
 // reconcileReplicaSet reconciles ReplicaSets
 type reconcileReplicaSet struct {
 	client client.Client
+	log    logr.Logger
 }
 
 // Implement reconcile.Reconciler so the controller can reconcile objects
 var _ reconcile.Reconciler = &reconcileReplicaSet{}
 
 func (r *reconcileReplicaSet) Reconcile(request reconcile.Request) (reconcile.Result, error) {
+	// set up a convinient log object so we don't have to type request over and over again
+	log := r.log.WithValues("request", request)
+
 	// Fetch the ReplicaSet from the cache
 	rs := &appsv1.ReplicaSet{}
 	err := r.client.Get(context.TODO(), request.NamespacedName, rs)
 	if errors.IsNotFound(err) {
-		log.Printf("Could not find ReplicaSet %v.\n", request)
+		log.Error(nil, "Could not find ReplicaSet")
 		return reconcile.Result{}, nil
 	}
 
 	if err != nil {
-		log.Printf("Could not fetch ReplicaSet %v for %+v\n", err, request)
+		log.Error(err, "Could not fetch ReplicaSet")
 		return reconcile.Result{}, err
 	}
 
 	// Print the ReplicaSet
-	log.Printf("ReplicaSet Name %s Namespace %s, Pod Name: %s\n",
-		rs.Name, rs.Namespace, rs.Spec.Template.Spec.Containers[0].Name)
+	log.Info("Reconciling ReplicaSet", "container name", rs.Spec.Template.Spec.Containers[0].Name)
 
 	// Set the label if it is missing
 	if rs.Labels == nil {
@@ -106,7 +122,7 @@ func (r *reconcileReplicaSet) Reconcile(request reconcile.Request) (reconcile.Re
 	rs.Labels["hello"] = "world"
 	err = r.client.Update(context.TODO(), rs)
 	if err != nil {
-		log.Printf("Could not write ReplicaSet %v\n", err)
+		log.Error(err, "Could not write ReplicaSet")
 		return reconcile.Result{}, err
 	}
 

hack/check-everything.sh

@@ -0,0 +1,79 @@
+#!/usr/bin/env bash
+
+#  Copyright 2018 The Kubernetes Authors.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+set -e
+
+hack_dir=$(dirname ${BASH_SOURCE})
+source ${hack_dir}/common.sh
+
+k8s_version=1.10.1
+goarch=amd64
+goos="unknown"
+
+if [[ "$OSTYPE" == "linux-gnu" ]]; then
+  goos="linux"
+elif [[ "$OSTYPE" == "darwin"* ]]; then
+  goos="darwin"
+fi
+
+if [[ "$goos" == "unknown" ]]; then
+  echo "OS '$OSTYPE' not supported. Aborting." >&2
+  exit 1
+fi
+
+tmp_root=/tmp
+kb_root_dir=$tmp_root/kubebuilder
+
+# Skip fetching and untaring the tools by setting the SKIP_FETCH_TOOLS variable
+# in your environment to any value:
+#
+# $ SKIP_FETCH_TOOLS=1 ./test.sh
+#
+# If you skip fetching tools, this script will use the tools already on your
+# machine, but rebuild the kubebuilder and kubebuilder-bin binaries.
+SKIP_FETCH_TOOLS=${SKIP_FETCH_TOOLS:-""}
+
+# fetch k8s API gen tools and make it available under kb_root_dir/bin.
+function fetch_kb_tools {
+  if [ -n "$SKIP_FETCH_TOOLS" ]; then
+    return 0
+  fi
+
+  header_text "fetching tools"
+  kb_tools_archive_name="kubebuilder-tools-$k8s_version-$goos-$goarch.tar.gz"
+  kb_tools_download_url="https://storage.googleapis.com/kubebuilder-tools/$kb_tools_archive_name"
+
+  kb_tools_archive_path="$tmp_root/$kb_tools_archive_name"
+  if [ ! -f $kb_tools_archive_path ]; then
+    curl -sL ${kb_tools_download_url} -o "$kb_tools_archive_path"
+  fi
+  tar -zvxf "$kb_tools_archive_path" -C "$tmp_root/"
+}
+
+header_text "using tools"
+
+which gometalinter.v2
+fetch_kb_tools
+setup_envs
+
+${hack_dir}/verify.sh
+${hack_dir}/test-all.sh
+
+header_text "confirming example compiles (via go install)"
+go install ./example
+
+echo "passed"
+exit 0

hack/common.sh

@@ -0,0 +1,52 @@
+#!/usr/bin/env bash
+
+#  Copyright 2018 The Kubernetes Authors.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+set -e
+
+# Enable tracing in this script off by setting the TRACE variable in your
+# environment to any value:
+#
+# $ TRACE=1 test.sh
+TRACE=${TRACE:-""}
+if [ -n "$TRACE" ]; then
+  set -x
+fi
+
+# Turn colors in this script off by setting the NO_COLOR variable in your
+# environment to any value:
+#
+# $ NO_COLOR=1 test.sh
+NO_COLOR=${NO_COLOR:-""}
+if [ -z "$NO_COLOR" ]; then
+  header=$'\e[1;33m'
+  reset=$'\e[0m'
+else
+  header=''
+  reset=''
+fi
+
+function header_text {
+  echo "$header$*$reset"
+}
+
+function setup_envs {
+  header_text "setting up env vars"
+
+  # Setup env vars
+  if [[ -z "${KUBEBUILDER_ASSETS}" ]]; then
+      export KUBEBUILDER_ASSETS=$kb_root_dir/bin
+  fi
+}

hack/test-all.sh

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash
+
+#  Copyright 2018 The Kubernetes Authors.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+set -e
+
+source $(dirname ${BASH_SOURCE})/common.sh
+
+setup_envs
+
+header_text "running go test"
+
+go test ./pkg/... -parallel 4
+
+header_text "running coverage"
+
+# Verify no coverage regressions have been introduced.  Remove the exception list from here
+# once the coverage has been brought back up
+if [[ ! $(go test ./pkg/...  -coverprofile cover.out -parallel 4 | grep -v "coverage: 100.0% of statements" | grep "controller-runtime/pkg " | grep -v "controller-runtime/pkg \|controller-runtime/pkg/recorder \|pkg/admission/certprovisioner \|pkg/internal/admission \|pkg/cache\|pkg/client \|pkg/event \|pkg/client/config \|pkg/controller/controllertest \|pkg/reconcile/reconciletest \|pkg/test ") ]]; then
+echo "ok"
+else
+go test ./pkg/...  -coverprofile cover.out -parallel 4 | grep -v "coverage: 100.0% of statements" | grep "controller-runtime/pkg " | grep -v "controller-runtime/pkg \|controller-runtime/pkg/recorder \|pkg/admission/certprovisioner \|pkg/internal/admission \|pkg/cache\|pkg/client \|pkg/event \|pkg/client/config \|pkg/controller/controllertest \|pkg/reconcile/reconciletest \|pkg/test "
+echo "missing test coverage"
+exit 1
+fi

hack/verify.sh

@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+
+#  Copyright 2018 The Kubernetes Authors.
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+
+set -e
+
+source $(dirname ${BASH_SOURCE})/common.sh
+
+header_text "running go vet"
+
+go vet ./pkg/...
+
+# go get is broken for golint.  re-enable this once it is fixed.
+#header_text "running golint"
+#
+#golint -set_exit_status ./pkg/...
+
+header_text "running gometalinter.v2"
+
+gometalinter.v2 --disable-all \
+    --deadline 5m \
+    --enable=misspell \
+    --enable=structcheck \
+    --enable=golint \
+    --enable=deadcode \
+    --enable=goimports \
+    --enable=errcheck \
+    --enable=varcheck \
+    --enable=goconst \
+    --enable=gas \
+    --enable=unparam \
+    --enable=ineffassign \
+    --enable=nakedret \
+    --enable=interfacer \
+    --enable=misspell \
+    --enable=gocyclo \
+    --line-length=170 \
+    --enable=lll \
+    --dupl-threshold=400 \
+    --enable=dupl \
+    --skip=atomic \
+    ./pkg/...
+# TODO: Enable these as we fix them to make them pass
+#    --enable=maligned \
+#    --enable=safesql \

pkg/admission/cert/writer/certwriter.go

@@ -23,7 +23,6 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
-	"log"
 	"net/url"
 	"time"
 
@@ -101,7 +100,7 @@ func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWrit
 	// Recreate the cert if it's invalid.
 	valid := validCert(certs, dnsName)
 	if !valid {
-		log.Printf("cert is invalid or expiring, regenerating a new one")
+		log.Info("cert is invalid or expiring, regenerating a new one")
 		certs, err = ch.overwrite(webhook.Name)
 		if err != nil {
 			return err

pkg/admission/cert/writer/doc.go

@@ -81,3 +81,9 @@ the desired path.
 
 */
 package writer
+
+import (
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+)
+
+var log = logf.KBLog.WithName("admission").WithName("cert").WithName("writer")