Repository: knative/pkg
Pull request #1443 (2020-06-24 09:31:31)

Assorted linting fixes.
- Error variables should start with "err" or "Err".
- Remove unnecessary `fmt.Sprint[f]`.
- Remove redundant `return`.
- Preallocate slices where possible.

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1443#" title="Author self-approved">markusthoemmes</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

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

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

- **[apis/OWNERS](https://github.com/knative/pkg/blob/master/apis/OWNERS)**
- **[controller/OWNERS](https://github.com/knative/pkg/blob/master/controller/OWNERS)**
- **[reconciler/OWNERS](https://github.com/knative/pkg/blob/master/reconciler/OWNERS)**
- **[test/OWNERS](https://github.com/knative/pkg/blob/master/test/OWNERS)**

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"]} -->
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
---- |:------------:|:------------:|:-----:
[reconciler/leader.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1443/pull-knative-pkg-go-coverage/1275723356607352833/artifacts/line-cov.html#file3) | 96.0% | 95.8% | -0.2

whaught: /lgtm
/approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1443#" title="Author self-approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1443#issuecomment-648866266" title="Approved">whaught</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

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

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

- **[apis/OWNERS](https://github.com/knative/pkg/blob/master/apis/OWNERS)**
- **[controller/OWNERS](https://github.com/knative/pkg/blob/master/controller/OWNERS)**
- **[reconciler/OWNERS](https://github.com/knative/pkg/blob/master/reconciler/OWNERS)**
- **[test/OWNERS](https://github.com/knative/pkg/blob/master/test/OWNERS)**

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"]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1443#" title="Author self-approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1443#pullrequestreview-436747365" title="Approved">matzew</a>*, *<a href="https://github.com/knative/pkg/pull/1443#issuecomment-648866266" title="Approved">whaught</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

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

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

- **[apis/OWNERS](https://github.com/knative/pkg/blob/master/apis/OWNERS)**
- **[controller/OWNERS](https://github.com/knative/pkg/blob/master/controller/OWNERS)**
- **[reconciler/OWNERS](https://github.com/knative/pkg/blob/master/reconciler/OWNERS)**
- **[test/OWNERS](https://github.com/knative/pkg/blob/master/test/OWNERS)**

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"]} -->

Patch:
diff --git a/apiextensions/storageversion/migrator.go b/apiextensions/storageversion/migrator.go
index 45a06ab92..657c30e3f 100644
--- a/apiextensions/storageversion/migrator.go
+++ b/apiextensions/storageversion/migrator.go
@@ -20,7 +20,7 @@ import (
 	"context"
 	"fmt"
 
-	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -54,7 +54,7 @@ func NewMigrator(d dynamic.Interface, a apixclient.Interface) *Migrator {
 // Finally the migrator will update the CRD's status and drop older storage
 // versions
 func (m *Migrator) Migrate(ctx context.Context, gr schema.GroupResource) error {
-	crdClient := m.apixClient.ApiextensionsV1beta1().CustomResourceDefinitions()
+	crdClient := m.apixClient.ApiextensionsV1().CustomResourceDefinitions()
 
 	crd, err := crdClient.Get(gr.String(), metav1.GetOptions{})
 	if err != nil {
@@ -63,6 +63,10 @@ func (m *Migrator) Migrate(ctx context.Context, gr schema.GroupResource) error {
 
 	version := storageVersion(crd)
 
+	if version == "" {
+		return fmt.Errorf("unable to determine storage version for %s", gr)
+	}
+
 	if err := m.migrateResources(ctx, gr.WithVersion(version)); err != nil {
 		return err
 	}
@@ -112,9 +116,5 @@ func storageVersion(crd *apix.CustomResourceDefinition) string {
 		}
 	}
 
-	if version == "" {
-		version = crd.Spec.Version
-	}
-
 	return version
 }
diff --git a/apis/field_error.go b/apis/field_error.go
index 3cedeecbb..37f1a6554 100644
--- a/apis/field_error.go
+++ b/apis/field_error.go
@@ -383,7 +383,7 @@ func ErrOutOfBoundsValue(value, lower, upper interface{}, fieldPath string) *Fie
 func CheckDisallowedFields(request, maskedRequest interface{}) *FieldError {
 	if disallowed, err := kmp.CompareSetFields(request, maskedRequest); err != nil {
 		return &FieldError{
-			Message: fmt.Sprintf("Internal Error"),
+			Message: "Internal Error",
 			Paths:   []string{CurrentField},
 		}
 	} else if len(disallowed) > 0 {
diff --git a/reconciler/leader.go b/reconciler/leader.go
index 3d5f68fdc..6fece8526 100644
--- a/reconciler/leader.go
+++ b/reconciler/leader.go
@@ -95,7 +95,6 @@ func (laf *LeaderAwareFuncs) Promote(b Bucket, enq func(Bucket, types.Namespaced
 			laf.buckets = make(map[string]Bucket, 1)
 		}
 		laf.buckets[b.Name()] = b
-		return
 	}()
 
 	if promote := laf.PromoteFunc; promote != nil {
diff --git a/webhook/admission.go b/webhook/admission.go
index 086f39dab..46d4c08c4 100644
--- a/webhook/admission.go
+++ b/webhook/admission.go
@@ -88,7 +88,13 @@ func admissionHandler(rootLogger *zap.SugaredLogger, stats StatsReporter, c Admi
 
 		ctx := logging.WithLogger(r.Context(), logger)
 
-		var response admissionv1.AdmissionReview
+		response := admissionv1.AdmissionReview{
+			// Use the same type meta as the request - this is required by the K8s API
+			// note: v1beta1 & v1 AdmissionReview shapes are identical so even though
+			// we're using v1 types we still support v1beta1 admission requests
+			TypeMeta: review.TypeMeta,
+		}
+
 		reviewResponse := c.Admit(ctx, review.Request)
 		var patchType string
 		if reviewResponse.PatchType != nil {


Test patch:
diff --git a/apiextensions/storageversion/migrator_test.go b/apiextensions/storageversion/migrator_test.go
index ff73b3b16..90c37bfff 100644
--- a/apiextensions/storageversion/migrator_test.go
+++ b/apiextensions/storageversion/migrator_test.go
@@ -22,7 +22,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixFake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
diff --git a/apis/duck/typed_test.go b/apis/duck/typed_test.go
index 08b693c78..4ef97e170 100644
--- a/apis/duck/typed_test.go
+++ b/apis/duck/typed_test.go
@@ -109,8 +109,8 @@ func TestInvalidResource(t *testing.T) {
 
 	_, _, got := tif.Get(SchemeGroupVersion.WithResource("resources"))
 
-	if got != testErr {
-		t.Errorf("Error = %v, want: %v", got, testErr)
+	if got != errTest {
+		t.Errorf("Error = %v, want: %v", got, errTest)
 	}
 }
 
@@ -296,7 +296,7 @@ func (bo *badObject) DeepCopyObject() runtime.Object {
 	return &badObject{}
 }
 
-var testErr = errors.New("failed to get list")
+var errTest = errors.New("failed to get list")
 
 type invalidResourceClient struct {
 	*fake.FakeDynamicClient
@@ -311,5 +311,5 @@ type invalidResource struct {
 }
 
 func (*invalidResource) List(options metav1.ListOptions) (*unstructured.UnstructuredList, error) {
-	return nil, testErr
+	return nil, errTest
 }
diff --git a/controller/controller_test.go b/controller/controller_test.go
index 8d8e14e72..3556af61a 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -933,8 +933,8 @@ func TestStartAndShutdownWithLeaderAwareWithLostElection(t *testing.T) {
 			Spec: coordinationv1.LeaseSpec{
 				HolderIdentity:       ptr.String("not-us"),
 				LeaseDurationSeconds: ptr.Int32(3000),
-				AcquireTime:          &metav1.MicroTime{time.Now()},
-				RenewTime:            &metav1.MicroTime{time.Now().Add(3000 * time.Second)},
+				AcquireTime:          &metav1.MicroTime{Time: time.Now()},
+				RenewTime:            &metav1.MicroTime{Time: time.Now().Add(3000 * time.Second)},
 			},
 		},
 	)
diff --git a/test/ghutil/fakeghutil/fakeghutil.go b/test/ghutil/fakeghutil/fakeghutil.go
index 7ac563580..51d8e2bd9 100644
--- a/test/ghutil/fakeghutil/fakeghutil.go
+++ b/test/ghutil/fakeghutil/fakeghutil.go
@@ -333,7 +333,7 @@ func (fgc *FakeGithubClient) CreatePullRequest(org, repo, head, base, title, bod
 
 // ListBranches lists branchs for given repo
 func (fgc *FakeGithubClient) ListBranches(org, repo string) ([]*github.Branch, error) {
-	var branches []*github.Branch
+	branches := make([]*github.Branch, 0, len(fgc.Branches))
 	for _, b := range fgc.Branches {
 		branches = append(branches, b...)
 	}
diff --git a/test/webhook-apicoverage/view/html_template.go b/test/webhook-apicoverage/view/html_template.go
index 89084c8fe..15539598d 100644
--- a/test/webhook-apicoverage/view/html_template.go
+++ b/test/webhook-apicoverage/view/html_template.go
@@ -16,11 +16,7 @@ limitations under the License.
 
 package view
 
-import (
-	"fmt"
-)
-
-var TypeCoverageTempl = fmt.Sprint(`<!DOCTYPE html>
+var TypeCoverageTempl = `<!DOCTYPE html>
 <html>
 <style type="text/css">
   <!--
@@ -81,9 +77,9 @@ var TypeCoverageTempl = fmt.Sprint(`<!DOCTYPE html>
 </table>
 </body>
 </html>
-`)
+`
 
-var AggregateCoverageTmpl = fmt.Sprint(`<!DOCTYPE html>
+var AggregateCoverageTmpl = `<!DOCTYPE html>
 <html>
 <style type="text/css">
   <!--
@@ -108,4 +104,4 @@ var AggregateCoverageTmpl = fmt.Sprint(`<!DOCTYPE html>
 </table>
 </body>
 </html>
-`)
+`
diff --git a/test/webhook-apicoverage/view/xml_template.go b/test/webhook-apicoverage/view/xml_template.go
index 019945e71..487a38078 100644
--- a/test/webhook-apicoverage/view/xml_template.go
+++ b/test/webhook-apicoverage/view/xml_template.go
@@ -16,11 +16,7 @@ limitations under the License.
 
 package view
 
-import (
-	"fmt"
-)
-
-var JunitResultTmpl = fmt.Sprint(`<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 }}
@@ -38,4 +34,4 @@ var JunitResultTmpl = fmt.Sprint(`<testsuites>
       </testcase>
     {{end}}
   </testsuite>
-</testsuites>`)
+</testsuites>`
diff --git a/webhook/admission_integration_test.go b/webhook/admission_integration_test.go
index 82be9bb1f..244124578 100644
--- a/webhook/admission_integration_test.go
+++ b/webhook/admission_integration_test.go
@@ -28,6 +28,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"golang.org/x/sync/errgroup"
 	jsonpatch "gomodules.xyz/jsonpatch/v2"
 	admissionv1 "k8s.io/api/admission/v1"
@@ -156,6 +157,11 @@ func TestAdmissionValidResponseForResource(t *testing.T) {
 			t.Errorf("Failed to decode response: %v", err)
 			return
 		}
+
+		if diff := cmp.Diff(rev.TypeMeta, reviewResponse.TypeMeta); diff != "" {
+			t.Errorf("expected the response typeMeta to be the same as the request (-want, +got)\n%s", diff)
+			return
+		}
 	}()
 
 	// Check that Admit calls block when they are initiated before informers sync.