Repository: knative/pkg
Pull request #1630 (2020-08-17 23:23:42)

Various cleanups around the codebase
- unindent the else after return
- make things private that are not used anywhere
- rearrange params
- etc

/assign mattmoor

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1630#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)~~ [vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[kmp/reporters.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1630/pull-knative-pkg-go-coverage/1295501958471225345/artifacts/line-cov.html#file2) | 95.2% | 92.9% | -2.4

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1630#pullrequestreview-468928480" title="Approved">mattmoor</a>*, *<a href="https://github.com/knative/pkg/pull/1630#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)~~ [mattmoor,vagababov]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
mattmoor: /hold

for the comment
knative-prow-robot: New changes are detected. LGTM label has been removed.
mattmoor: /lgtm
/unhold
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[kmp/reporters.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1630/pull-knative-pkg-go-coverage/1295532156394672128/artifacts/line-cov.html#file2) | 95.2% | 92.9% | -2.4

knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 784585eecf033d82c2839235c2f0eb915265afea | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1630/pull-knative-pkg-unit-tests/1295532156390477826) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1630). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
vagababov: Duh, I need to fix this test
/retest

Patch:
diff --git a/apis/duck/v1beta1/destination.go b/apis/duck/v1beta1/destination.go
index 9c247b030..fe0f5d171 100644
--- a/apis/duck/v1beta1/destination.go
+++ b/apis/duck/v1beta1/destination.go
@@ -107,9 +107,8 @@ func ValidateDestination(dest Destination, allowDeprecatedFields bool) *apis.Fie
 	if ref != nil && dest.URI == nil {
 		if dest.Ref != nil {
 			return validateDestinationRef(*ref).ViaField("ref")
-		} else {
-			return validateDestinationRef(*ref)
 		}
+		return validateDestinationRef(*ref)
 	}
 	return nil
 }
diff --git a/kmp/diff.go b/kmp/diff.go
index 09c041446..791c9d1d4 100644
--- a/kmp/diff.go
+++ b/kmp/diff.go
@@ -73,7 +73,7 @@ func SafeEqual(x, y interface{}, opts ...cmp.Option) (equal bool, err error) {
 // CompareSetFields returns a list of field names that differ between
 // x and y. Uses SafeEqual for comparison.
 func CompareSetFields(x, y interface{}, opts ...cmp.Option) ([]string, error) {
-	r := new(FieldListReporter)
+	r := new(fieldListReporter)
 	opts = append(opts, cmp.Reporter(r))
 	_, err := SafeEqual(x, y, opts...)
 	return r.Fields(), err
@@ -82,7 +82,7 @@ func CompareSetFields(x, y interface{}, opts ...cmp.Option) ([]string, error) {
 // ShortDiff returns a zero-context, unified human-readable diff.
 // Uses SafeEqual for comparison.
 func ShortDiff(prev, cur interface{}, opts ...cmp.Option) (string, error) {
-	r := new(ShortDiffReporter)
+	r := new(shortDiffReporter)
 	opts = append(opts, cmp.Reporter(r))
 	var err error
 	if _, err = SafeEqual(prev, cur, opts...); err != nil {
diff --git a/kmp/reporters.go b/kmp/reporters.go
index 5586f90bc..6f8ba0145 100644
--- a/kmp/reporters.go
+++ b/kmp/reporters.go
@@ -25,42 +25,41 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-// FieldListReporter implements the cmp.Reporter interface. It keeps
+// fieldListReporter implements the cmp.Reporter interface. It keeps
 // track of the field names that differ between two structs and reports
 // them through the Fields() function.
-type FieldListReporter struct {
+type fieldListReporter struct {
 	path       cmp.Path
 	fieldNames []string
 }
 
 // PushStep implements the cmp.Reporter.
-func (r *FieldListReporter) PushStep(ps cmp.PathStep) {
+func (r *fieldListReporter) PushStep(ps cmp.PathStep) {
 	r.path = append(r.path, ps)
 }
 
 // fieldName returns a readable name for the field. If the field has JSON annotations it
 // returns the JSON key. If the field does not have JSON annotations or the JSON annotation
 // marks the field as ignored it returns the field's go name
-func (r *FieldListReporter) fieldName() string {
+func (r *fieldListReporter) fieldName() string {
 	if len(r.path) < 2 {
 		return r.path.Index(0).String()
-	} else {
-		fieldName := strings.TrimPrefix(r.path.Index(1).String(), ".")
-		// Prefer JSON name to fieldName if it exists
-		structField, exists := r.path.Index(0).Type().FieldByName(fieldName)
-		if exists {
-			tag := structField.Tag.Get("json")
-			if tag != "" && tag != "-" {
-				return strings.SplitN(tag, ",", 2)[0]
-			}
-
+	}
+	fieldName := strings.TrimPrefix(r.path.Index(1).String(), ".")
+	// Prefer JSON name to fieldName if it exists
+	structField, exists := r.path.Index(0).Type().FieldByName(fieldName)
+	if exists {
+		tag := structField.Tag.Get("json")
+		if tag != "" && tag != "-" {
+			return strings.SplitN(tag, ",", 2)[0]
 		}
-		return fieldName
 	}
+
+	return fieldName
 }
 
 // Report implements the cmp.Reporter.
-func (r *FieldListReporter) Report(rs cmp.Result) {
+func (r *fieldListReporter) Report(rs cmp.Result) {
 	if rs.Equal() {
 		return
 	}
@@ -75,72 +74,70 @@ func (r *FieldListReporter) Report(rs cmp.Result) {
 }
 
 // PopStep implements cmp.Reporter.
-func (r *FieldListReporter) PopStep() {
+func (r *fieldListReporter) PopStep() {
 	r.path = r.path[:len(r.path)-1]
 }
 
 // Fields returns the field names that differed between the two
 // objects after calling cmp.Equal with the FieldListReporter. Field names
 // are returned in alphabetical order.
-func (r *FieldListReporter) Fields() []string {
+func (r *fieldListReporter) Fields() []string {
 	sort.Strings(r.fieldNames)
 	return r.fieldNames
 }
 
-// ShortDiffReporter implements the cmp.Reporter interface. It reports
+// shortDiffReporter implements the cmp.Reporter interface. It reports
 // on fields which have diffing values in a short zero-context, unified diff
 // format.
-type ShortDiffReporter struct {
+type shortDiffReporter struct {
 	path  cmp.Path
 	diffs []string
 	err   error
 }
 
 // PushStep implements the cmp.Reporter.
-func (r *ShortDiffReporter) PushStep(ps cmp.PathStep) {
+func (r *shortDiffReporter) PushStep(ps cmp.PathStep) {
 	r.path = append(r.path, ps)
 }
 
 // Report implements the cmp.Reporter.
-func (r *ShortDiffReporter) Report(rs cmp.Result) {
+func (r *shortDiffReporter) Report(rs cmp.Result) {
 	if rs.Equal() {
 		return
 	}
 	cur := r.path.Last()
 	vx, vy := cur.Values()
 	t := cur.Type()
-	var diff string
 	// Prefix struct values with the types to add clarity in output
 	if !vx.IsValid() && !vy.IsValid() {
 		r.err = fmt.Errorf("unable to diff %+v and %+v on path %#v", vx, vy, r.path)
-	} else {
-		diff = fmt.Sprintf("%#v:\n", r.path)
-		if vx.IsValid() {
-			diff += r.diffString("-", t, vx)
-		}
-		if vy.IsValid() {
-			diff += r.diffString("+", t, vy)
-		}
+		return
+	}
+	diff := fmt.Sprintf("%#v:\n", r.path)
+	if vx.IsValid() {
+		diff += r.diffString("-", t, vx)
+	}
+	if vy.IsValid() {
+		diff += r.diffString("+", t, vy)
 	}
 	r.diffs = append(r.diffs, diff)
 }
 
-func (r *ShortDiffReporter) diffString(diffType string, t reflect.Type, v reflect.Value) string {
+func (r *shortDiffReporter) diffString(diffType string, t reflect.Type, v reflect.Value) string {
 	if t.Kind() == reflect.Struct {
 		return fmt.Sprintf("\t%s: %+v: \"%+v\"\n", diffType, t, v)
-	} else {
-		return fmt.Sprintf("\t%s: \"%+v\"\n", diffType, v)
 	}
+	return fmt.Sprintf("\t%s: \"%+v\"\n", diffType, v)
 }
 
 // PopStep implements the cmp.Reporter.
-func (r *ShortDiffReporter) PopStep() {
+func (r *shortDiffReporter) PopStep() {
 	r.path = r.path[:len(r.path)-1]
 }
 
 // Diff returns the generated short diff for this object.
 // cmp.Equal should be called before this method.
-func (r *ShortDiffReporter) Diff() (string, error) {
+func (r *shortDiffReporter) Diff() (string, error) {
 	if r.err != nil {
 		return "", r.err
 	}
diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go
index faebc5f6e..9c82de137 100644
--- a/webhook/resourcesemantics/defaulting/defaulting.go
+++ b/webhook/resourcesemantics/defaulting/defaulting.go
@@ -279,7 +279,7 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1.AdmissionRequ
 
 		s, ok := oldObj.(apis.HasSpec)
 		if ok {
-			SetUserInfoAnnotations(s, ctx, req.Resource.Group)
+			setUserInfoAnnotations(ctx, s, req.Resource.Group)
 		}
 
 		if req.SubResource == "" {
@@ -323,7 +323,7 @@ func (ac *reconciler) setUserInfoAnnotations(ctx context.Context, patches duck.J
 
 	b, a := new.DeepCopyObject().(apis.HasSpec), nh
 
-	SetUserInfoAnnotations(nh, ctx, groupName)
+	setUserInfoAnnotations(ctx, nh, groupName)
 
 	patch, err := duck.CreatePatch(b, a)
 	if err != nil {
diff --git a/webhook/resourcesemantics/defaulting/user_info.go b/webhook/resourcesemantics/defaulting/user_info.go
index a1243b8e8..36c1cfbd2 100644
--- a/webhook/resourcesemantics/defaulting/user_info.go
+++ b/webhook/resourcesemantics/defaulting/user_info.go
@@ -24,8 +24,8 @@ import (
 	"knative.dev/pkg/apis"
 )
 
-// SetUserInfoAnnotations sets creator and updater annotations on a resource.
-func SetUserInfoAnnotations(resource apis.HasSpec, ctx context.Context, groupName string) {
+// setUserInfoAnnotations sets creator and updater annotations on a resource.
+func setUserInfoAnnotations(ctx context.Context, resource apis.HasSpec, groupName string) {
 	if ui := apis.GetUserInfo(ctx); ui != nil {
 		objectMetaAccessor, ok := resource.(metav1.ObjectMetaAccessor)
 		if !ok {


Test patch:
diff --git a/kmp/reporters_test.go b/kmp/reporters_test.go
index 2daaf42d7..c8b36667d 100644
--- a/kmp/reporters_test.go
+++ b/kmp/reporters_test.go
@@ -183,7 +183,7 @@ func TestFieldListReporter(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			reporter := new(FieldListReporter)
+			reporter := new(fieldListReporter)
 			cmp.Equal(test.x, test.y, append(test.opts, cmp.Reporter(reporter))...)
 			got := reporter.Fields()
 			if diff := cmp.Diff(got, test.want); diff != "" {
@@ -343,7 +343,7 @@ func TestImmutableReporter(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			reporter := new(ShortDiffReporter)
+			reporter := new(shortDiffReporter)
 			cmp.Equal(test.x, test.y, append(test.opts, cmp.Reporter(reporter))...)
 			got, err := reporter.Diff()
 			if test.expectErr {
diff --git a/test/logging/error.go b/test/logging/error.go
index d3caea91c..e34916075 100644
--- a/test/logging/error.go
+++ b/test/logging/error.go
@@ -50,15 +50,14 @@ func keysAndValuesToSpewedMap(args ...interface{}) map[string]string {
 	return m
 }
 
-// Implement `error` interface
+// Error implements `error` interface
 func (e structuredError) Error() string {
 	// TODO(coryrc): accept zap.Field entries?
 	if e.print {
 		// %v for fmt.Sprintf does print keys sorted
 		return fmt.Sprintf("Error: %s\nContext:\n%v", e.msg, keysAndValuesToSpewedMap(e.keysAndValues...))
-	} else {
-		return e.msg
 	}
+	return e.msg
 }
 
 // GetValues gives you the structured key values in a plist
diff --git a/test/logging/tlogger.go b/test/logging/tlogger.go
index e89e2294a..c497d7384 100644
--- a/test/logging/tlogger.go
+++ b/test/logging/tlogger.go
@@ -163,22 +163,21 @@ func (o *TLogger) interfacesToFields(things ...interface{}) []interface{} {
 func (o *TLogger) errorWithRuntimeCheck(stringThenKeysAndValues ...interface{}) (error, string, []interface{}) {
 	if len(stringThenKeysAndValues) == 0 {
 		return nil, "", nil
-	} else {
-		s, isString := stringThenKeysAndValues[0].(string)
-		e, isError := stringThenKeysAndValues[0].(error)
-		if isString {
-			// Desired case (hopefully)
-			remainder := stringThenKeysAndValues[1:]
-			if !validateKeysAndValues(remainder...) {
-				remainder = o.interfacesToFields(remainder...)
-			}
-			return nil, s, remainder
-		} else if isError && len(stringThenKeysAndValues) == 1 {
-			return e, "", nil
-		} else {
-			return nil, "unstructured error", o.interfacesToFields(stringThenKeysAndValues...)
+	}
+	s, isString := stringThenKeysAndValues[0].(string)
+	if isString {
+		// Desired case (hopefully)
+		remainder := stringThenKeysAndValues[1:]
+		if !validateKeysAndValues(remainder...) {
+			remainder = o.interfacesToFields(remainder...)
 		}
+		return nil, s, remainder
+	}
+	e, isError := stringThenKeysAndValues[0].(error)
+	if isError && len(stringThenKeysAndValues) == 1 {
+		return e, "", nil
 	}
+	return nil, "unstructured error", o.interfacesToFields(stringThenKeysAndValues...)
 }
 
 // Cleanup registers a cleanup callback.
diff --git a/test/webhook-apicoverage/view/xml_display.go b/test/webhook-apicoverage/view/xml_display.go
index 146312a9a..159c6528e 100644
--- a/test/webhook-apicoverage/view/xml_display.go
+++ b/test/webhook-apicoverage/view/xml_display.go
@@ -27,7 +27,7 @@ import (
 // percentage values to junit xml file format.
 func GetCoveragePercentageXMLDisplay(
 	percentageCoverages *coveragecalculator.CoveragePercentages) (string, error) {
-	tmpl, err := template.New("JunitResult").Parse(JunitResultTmpl)
+	tmpl, err := template.New("JunitResult").Parse(junitResultTmpl)
 	if err != nil {
 		return "", err
 	}
diff --git a/test/webhook-apicoverage/view/xml_template.go b/test/webhook-apicoverage/view/xml_template.go
index 487a38078..36c97ad28 100644
--- a/test/webhook-apicoverage/view/xml_template.go
+++ b/test/webhook-apicoverage/view/xml_template.go
@@ -16,7 +16,7 @@ limitations under the License.
 
 package view
 
-var JunitResultTmpl = `<testsuites>
+var junitResultTmpl = `<testsuites>
   <testsuite name="" time="0" {{ if .IsFailedBuild }} failures="1" {{ else }} failures = "0" {{ end }} tests="0">
       <testcase name="Overall" time="0" classname="go_coverage">
 				{{ if .IsFailedBuild }}
diff --git a/testutils/clustermanager/e2e-tests/boskos/fake/fake.go b/testutils/clustermanager/e2e-tests/boskos/fake/fake.go
index 988b409dd..4c427501c 100644
--- a/testutils/clustermanager/e2e-tests/boskos/fake/fake.go
+++ b/testutils/clustermanager/e2e-tests/boskos/fake/fake.go
@@ -65,12 +65,11 @@ func (c *FakeBoskosClient) ReleaseGKEProject(name string) error {
 				res.Owner = ""
 				res.State = boskoscommon.Free
 				return nil
-			} else {
-				return fmt.Errorf("Got owner: '%s', expect owner: '%s'", res.Owner, owner)
 			}
+			return fmt.Errorf("Got owner: %q, expect owner: %q", res.Owner, owner)
 		}
 	}
-	return fmt.Errorf("resource doesn't exist yet: '%s'", name)
+	return fmt.Errorf("resource doesn't exist yet: %q", name)
 }
 
 // NewGKEProject adds Boskos resources for testing purpose
diff --git a/webhook/resourcesemantics/defaulting/user_info_test.go b/webhook/resourcesemantics/defaulting/user_info_test.go
index 89ef5ce42..18c3eb0a2 100644
--- a/webhook/resourcesemantics/defaulting/user_info_test.go
+++ b/webhook/resourcesemantics/defaulting/user_info_test.go
@@ -78,7 +78,7 @@ func TestSetUserInfoAnnotationsWhenWithinCreate(t *testing.T) {
 
 			tc.setup(ctx, r)
 
-			SetUserInfoAnnotations(r, ctx, "pkg.knative.dev")
+			setUserInfoAnnotations(ctx, r, "pkg.knative.dev")
 
 			if !reflect.DeepEqual(r.Annotations, tc.expectedAnnotations) {
 				t.Logf("Got :  %#v", r.Annotations)
@@ -158,21 +158,20 @@ func TestSetUserInfoAnnotationsWhenWithinUpdate(t *testing.T) {
 
 			ctx := tc.configureContext(TestContextWithLogger(t), r)
 
-			new := r.DeepCopy()
+			newObj := r.DeepCopy()
 
-			tc.setup(ctx, new)
+			tc.setup(ctx, newObj)
 
-			SetUserInfoAnnotations(new, ctx, "pkg.knative.dev")
+			setUserInfoAnnotations(ctx, newObj, "pkg.knative.dev")
 
-			if !reflect.DeepEqual(new.Annotations, tc.expectedAnnotations) {
-				t.Logf("Got :  %#v", new.Annotations)
+			if !reflect.DeepEqual(newObj.Annotations, tc.expectedAnnotations) {
+				t.Logf("Got :  %#v", newObj.Annotations)
 				t.Logf("Want: %#v", tc.expectedAnnotations)
-				if diff := cmp.Diff(tc.expectedAnnotations, new.Annotations, cmpopts.EquateEmpty()); diff != "" {
-					t.Logf("diff: %v", diff)
+				if diff := cmp.Diff(tc.expectedAnnotations, newObj.Annotations, cmpopts.EquateEmpty()); diff != "" {
+					t.Logf("diff:\n%s", diff)
 				}
 				t.Fatalf("Annotations don't match")
 			}
-
 		})
 	}
 }
diff --git a/webhook/resourcesemantics/validation/validation_admit_test.go b/webhook/resourcesemantics/validation/validation_admit_test.go
index 47dc65522..d225369b2 100644
--- a/webhook/resourcesemantics/validation/validation_admit_test.go
+++ b/webhook/resourcesemantics/validation/validation_admit_test.go
@@ -278,9 +278,8 @@ func resourceCallback(ctx context.Context, uns *unstructured.Unstructured) error
 	if apis.IsInDelete(ctx) {
 		if resource.Spec.FieldForCallbackValidation != "magic delete" {
 			return errors.New("no magic delete")
-		} else {
-			return nil
 		}
+		return nil
 	}
 
 	if apis.IsDryRun(ctx) {