From ea75ee32c0299bbd541b786a2b7d032b8ec3408f Mon Sep 17 00:00:00 2001 From: RainbowMango Date: Tue, 27 Aug 2019 10:44:34 +0800 Subject: [PATCH 1/2] Fix staticcheck issues: Dealing with unused functions/variables/types. (staticcheck U1000) Dealing with value never used issue. (staticcheck SA4006) Dealing with concurrency issue. (staticcheck SA2002 SA4010) Remove packages from staticcheck failure files: apiextensions-apiserver --- hack/.staticcheck_failures | 10 --------- .../apiserver/conversion/webhook_converter.go | 10 --------- .../apiserver/customresource_handler_test.go | 3 +++ .../pkg/apiserver/schema/goopenapi_test.go | 3 --- .../schema/objectmeta/validation_test.go | 12 ---------- .../schema/pruning/algorithm_test.go | 1 + .../pkg/controller/finalizer/crd_finalizer.go | 3 +-- .../pkg/registry/customresource/etcd.go | 2 +- .../test/integration/basic_test.go | 9 ++++++-- .../test/integration/change_test.go | 22 ++++++++++++------- .../integration/conversion/conversion_test.go | 5 +---- .../test/integration/fixtures/server.go | 3 +++ .../test/integration/subresources_test.go | 2 +- .../test/integration/validation_test.go | 9 +++----- 14 files changed, 35 insertions(+), 59 deletions(-) diff --git a/hack/.staticcheck_failures b/hack/.staticcheck_failures index fc1300206df..0f5fc2c27d2 100644 --- a/hack/.staticcheck_failures +++ b/hack/.staticcheck_failures @@ -137,16 +137,6 @@ test/integration/ttlcontroller test/integration/volume test/integration/volumescheduling test/utils -vendor/k8s.io/apiextensions-apiserver/pkg/apiserver -vendor/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion -vendor/k8s.io/apiextensions-apiserver/pkg/apiserver/schema -vendor/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta -vendor/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning -vendor/k8s.io/apiextensions-apiserver/pkg/controller/finalizer -vendor/k8s.io/apiextensions-apiserver/pkg/registry/customresource -vendor/k8s.io/apiextensions-apiserver/test/integration -vendor/k8s.io/apiextensions-apiserver/test/integration/conversion -vendor/k8s.io/apiextensions-apiserver/test/integration/fixtures vendor/k8s.io/apimachinery/pkg/api/meta vendor/k8s.io/apimachinery/pkg/api/resource vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go index fabc8b3f6f6..8013e89767b 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/webhook_converter.go @@ -112,16 +112,6 @@ func (f *webhookConverterFactory) NewWebhookConverter(crd *internal.CustomResour }, nil } -// hasConversionReviewVersion check whether a version is accepted by a given webhook. -func (c *webhookConverter) hasConversionReviewVersion(v string) bool { - for _, b := range c.conversionReviewVersions { - if b == v { - return true - } - } - return false -} - // getObjectsToConvert returns a list of objects requiring conversion. // if obj is a list, getObjectsToConvert returns a (potentially empty) list of the items that are not already in the desired version. // if obj is not a list, and is already in the desired version, getObjectsToConvert returns an empty list. diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go index b5301530936..9d1db5d27f3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler_test.go @@ -96,6 +96,9 @@ func TestConvertFieldLabel(t *testing.T) { t.Fatal(err) } _, c, err := f.NewConverter(&crd) + if err != nil { + t.Fatalf("Failed to create CR converter. error: %v", err) + } label, value, err := c.ConvertFieldLabel(schema.GroupVersionKind{}, test.label, "value") if e, a := test.expectError, err != nil; e != a { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go index 3b13e16e4cd..8106312a8b2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/goopenapi_test.go @@ -19,7 +19,6 @@ package schema import ( "math/rand" "reflect" - "regexp" "testing" "time" @@ -31,8 +30,6 @@ import ( "k8s.io/apimachinery/pkg/util/json" ) -var nullTypeRE = regexp.MustCompile(`"type":\["([^"]*)","null"]`) - func TestStructuralRoundtrip(t *testing.T) { f := fuzz.New() seed := time.Now().UnixNano() diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation_test.go index bed35140be1..4270bcd7c10 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation_test.go @@ -225,18 +225,6 @@ func required(path ...string) validationMatch { func invalid(path ...string) validationMatch { return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} } -func invalidIndex(index int, path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...).Index(index), errorType: field.ErrorTypeInvalid} -} -func unsupported(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeNotSupported} -} -func immutable(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} -} -func forbidden(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeForbidden} -} func (v validationMatch) matches(err *field.Error) bool { return err.Type == v.errorType && err.Field == v.path.String() diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm_test.go index 4ddf9301144..b7b5ec7604d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm_test.go @@ -636,6 +636,7 @@ func BenchmarkDeepCopy(b *testing.B) { b.StartTimer() for i := 0; i < b.N; i++ { + //lint:ignore SA4010 the result of append is never used, it's acceptable since in benchmark testing. instances = append(instances, runtime.DeepCopyJSON(obj)) } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index dcf4a4e7717..6483adf973c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -149,8 +149,7 @@ func (c *CRDFinalizer) sync(key string) error { cond, deleteErr := c.deleteInstances(crd) apiextensions.SetCRDCondition(crd, cond) if deleteErr != nil { - crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) - if err != nil { + if _, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd); err != nil { utilruntime.HandleError(err) } return deleteErr diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go index ef4c4f84eeb..f8261196407 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/etcd.go @@ -342,7 +342,7 @@ func scaleFromCustomResource(cr *unstructured.Unstructured, specReplicasPath, st var labelSelector string if len(labelSelectorPath) > 0 { labelSelectorPath = strings.TrimPrefix(labelSelectorPath, ".") // ignore leading period - labelSelector, found, err = unstructured.NestedString(cr.UnstructuredContent(), strings.Split(labelSelectorPath, ".")...) + labelSelector, _, err = unstructured.NestedString(cr.UnstructuredContent(), strings.Split(labelSelectorPath, ".")...) if err != nil { return nil, false, err } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go index b8ef583730d..9ae4c0e42a1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/basic_test.go @@ -524,8 +524,7 @@ func TestDiscovery(t *testing.T) { scope := apiextensionsv1beta1.NamespaceScoped noxuDefinition := fixtures.NewNoxuCustomResourceDefinition(scope) - noxuDefinition, err = fixtures.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient) - if err != nil { + if _, err = fixtures.CreateNewCustomResourceDefinition(noxuDefinition, apiExtensionClient, dynamicClient); err != nil { t.Fatal(err) } @@ -828,12 +827,18 @@ func TestCrossNamespaceListWatch(t *testing.T) { noxuNamespacedResourceClient1 := newNamespacedCustomResourceClient(ns1, dynamicClient, noxuDefinition) instances[ns1] = createInstanceWithNamespaceHelper(t, ns1, "foo1", noxuNamespacedResourceClient1, noxuDefinition) noxuNamespacesWatch1, err := noxuNamespacedResourceClient1.Watch(metav1.ListOptions{ResourceVersion: initialListListMeta.GetResourceVersion()}) + if err != nil { + t.Fatalf("Failed to watch namespace: %s, error: %v", ns1, err) + } defer noxuNamespacesWatch1.Stop() ns2 := "namespace-2" noxuNamespacedResourceClient2 := newNamespacedCustomResourceClient(ns2, dynamicClient, noxuDefinition) instances[ns2] = createInstanceWithNamespaceHelper(t, ns2, "foo2", noxuNamespacedResourceClient2, noxuDefinition) noxuNamespacesWatch2, err := noxuNamespacedResourceClient2.Watch(metav1.ListOptions{ResourceVersion: initialListListMeta.GetResourceVersion()}) + if err != nil { + t.Fatalf("Failed to watch namespace: %s, error: %v", ns2, err) + } defer noxuNamespacesWatch2.Stop() createdList, err := noxuResourceClient.List(metav1.ListOptions{}) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/change_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/change_test.go index 3e0b4bc3e6c..46e56f1b426 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/change_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/change_test.go @@ -73,9 +73,12 @@ func TestChangeCRD(t *testing.T) { default: } + time.Sleep(10 * time.Millisecond) + noxuDefinitionToUpdate, err := apiExtensionsClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get(noxuDefinition.Name, metav1.GetOptions{}) if err != nil { - t.Fatal(err) + t.Error(err) + continue } if len(noxuDefinitionToUpdate.Spec.Versions) == 1 { v2 := noxuDefinitionToUpdate.Spec.Versions[0] @@ -87,9 +90,9 @@ func TestChangeCRD(t *testing.T) { noxuDefinitionToUpdate.Spec.Versions = noxuDefinitionToUpdate.Spec.Versions[0:1] } if _, err := apiExtensionsClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(noxuDefinitionToUpdate); err != nil && !apierrors.IsConflict(err) { - t.Fatal(err) + t.Error(err) + continue } - time.Sleep(10 * time.Millisecond) } }() @@ -100,18 +103,20 @@ func TestChangeCRD(t *testing.T) { defer wg.Done() noxuInstanceToCreate := fixtures.NewNoxuInstance(ns, fmt.Sprintf("foo-%d", i)) if _, err := noxuNamespacedResourceClient.Create(noxuInstanceToCreate, metav1.CreateOptions{}); err != nil { - t.Fatal(err) + t.Error(err) + return } for { + time.Sleep(10 * time.Millisecond) select { case <-stopChan: return default: if _, err := noxuNamespacedResourceClient.Get(noxuInstanceToCreate.GetName(), metav1.GetOptions{}); err != nil { - t.Fatal(err) + t.Error(err) + continue } } - time.Sleep(10 * time.Millisecond) } }(i) @@ -119,13 +124,15 @@ func TestChangeCRD(t *testing.T) { go func(i int) { defer wg.Done() for { + time.Sleep(10 * time.Millisecond) select { case <-stopChan: return default: w, err := noxuNamespacedResourceClient.Watch(metav1.ListOptions{}) if err != nil { - t.Fatalf("unexpected error establishing watch: %v", err) + t.Errorf("unexpected error establishing watch: %v", err) + continue } for event := range w.ResultChan() { switch event.Type { @@ -136,7 +143,6 @@ func TestChangeCRD(t *testing.T) { } } } - time.Sleep(10 * time.Millisecond) } }(i) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index 107d0e67cdf..8955921a5f7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -45,7 +45,6 @@ import ( apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" - "k8s.io/apiextensions-apiserver/pkg/cmd/server/options" serveroptions "k8s.io/apiextensions-apiserver/pkg/cmd/server/options" apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" "k8s.io/apiextensions-apiserver/test/integration/fixtures" @@ -271,8 +270,7 @@ func validateStorageVersion(t *testing.T, ctc *conversionTestContext) { } ctc.setAndWaitStorageVersion(t, "v1beta2") - obj, err = client.Get(obj.GetName(), metav1.GetOptions{}) - if err != nil { + if _, err = client.Get(obj.GetName(), metav1.GetOptions{}); err != nil { t.Fatal(err) } @@ -942,7 +940,6 @@ type conversionTestContext struct { namespace string apiExtensionsClient clientset.Interface dynamicClient dynamic.Interface - options *options.CustomResourceDefinitionsServerOptions crd *apiextensionsv1beta1.CustomResourceDefinition etcdObjectReader *storage.EtcdObjectReader } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go index d2f487051fc..4678bc04f1d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go @@ -34,6 +34,9 @@ import ( func StartDefaultServer(t servertesting.Logger, flags ...string) (func(), *rest.Config, *options.CustomResourceDefinitionsServerOptions, error) { // create kubeconfig which will not actually be used. But authz/authn needs it to startup. fakeKubeConfig, err := ioutil.TempFile("", "kubeconfig") + if err != nil { + return nil, nil, nil, err + } fakeKubeConfig.WriteString(` apiVersion: v1 kind: Config diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go index bafcaa28404..4c0ad1dde2e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go @@ -536,7 +536,7 @@ func TestValidateOnlyStatus(t *testing.T) { if err != nil { t.Fatalf("unexpected error setting .status.num: %v", err) } - createdNoxuInstance, err = noxuResourceClient.UpdateStatus(createdNoxuInstance, metav1.UpdateOptions{}) + _, err = noxuResourceClient.UpdateStatus(createdNoxuInstance, metav1.UpdateOptions{}) if err == nil { t.Fatal("expected error, but got none") } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index caea620b144..b1e17246256 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -582,8 +582,7 @@ spec: // create CRDs t.Logf("Creating CRD %s", crd.Name) - crd, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Create(crd) - if err != nil { + if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Create(crd); err != nil { t.Fatalf("unexpected create error: %v", err) } @@ -613,8 +612,7 @@ spec: t.Fatalf("unexpected get error: %v", err) } crd.Spec.Validation = nil - crd, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(crd) - if apierrors.IsConflict(err) { + if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(crd); apierrors.IsConflict(err) { continue } if err != nil { @@ -647,8 +645,7 @@ spec: t.Fatalf("unexpected get error: %v", err) } crd.Spec.Validation = &apiextensionsv1beta1.CustomResourceValidation{OpenAPIV3Schema: origSchema} - crd, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(crd) - if apierrors.IsConflict(err) { + if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(crd); apierrors.IsConflict(err) { continue } if err != nil { From 31b08c849186a78d680f099ee6076c2e23f373c2 Mon Sep 17 00:00:00 2001 From: RainbowMango Date: Wed, 25 Sep 2019 09:50:56 +0800 Subject: [PATCH 2/2] Fix a new staticcheck issue. vendor/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go:167:2: this value of crd is never used (SA4006) --- .../pkg/controller/finalizer/crd_finalizer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go index 6483adf973c..81330540d7c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/finalizer/crd_finalizer.go @@ -164,7 +164,7 @@ func (c *CRDFinalizer) sync(key string) error { } apiextensions.CRDRemoveFinalizer(crd, apiextensions.CustomResourceCleanupFinalizer) - crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) + _, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) if apierrors.IsNotFound(err) || apierrors.IsConflict(err) { // deleted or changed in the meantime, we'll get called again return nil