implement review findings

Signed-off-by: Tobias Giese <[email protected]>
NVIDIA · Nov 11, 2024 · b36bdfc · b36bdfc
1 parent 31e447f
commit b36bdfc
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 30 deletions.
diff --git a/cmd/apply-crds/main.go b/cmd/apply-crds/main.go
@@ -28,6 +28,7 @@ import (
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	v1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -85,13 +86,13 @@ func Run() {
 		log.Fatalf("Failed to create API extensions client: %v", err)
 	}
 
-	if err := walkCrdsDir(ctx, client); err != nil {
+	if err := walkCrdsDir(ctx, client.ApiextensionsV1().CustomResourceDefinitions()); err != nil {
 		log.Fatalf("Failed to apply CRDs: %v", err)
 	}
 }
 
 // walkCrdsDir walks the CRDs directory and applies each YAML file.
-func walkCrdsDir(ctx context.Context, client *clientset.Clientset) error {
+func walkCrdsDir(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface) error {
 	for _, crdDir := range crdsDir {
 		// Walk the directory recursively and apply each YAML file.
 		err := filepath.Walk(crdDir, func(path string, info os.FileInfo, err error) error {
@@ -103,23 +104,23 @@ func walkCrdsDir(ctx context.Context, client *clientset.Clientset) error {
 			}
 
 			log.Printf("Apply CRDs from file: %s", path)
-			if err := applyCRDsFromFile(ctx, client, path); err != nil {
-				return fmt.Errorf("apply CRD %s: %v", path, err)
+			if err := applyCRDsFromFile(ctx, crdClient, path); err != nil {
+				return fmt.Errorf("apply CRD %s: %w", path, err)
 			}
 			return nil
 		})
 		if err != nil {
-			return fmt.Errorf("walk the path %s: %v", crdsDir, err)
+			return fmt.Errorf("walk the path %s: %w", crdsDir, err)
 		}
 	}
 	return nil
 }
 
 // applyCRDsFromFile reads a YAML file, splits it into documents, and applies each CRD to the cluster.
-func applyCRDsFromFile(ctx context.Context, client *clientset.Clientset, filePath string) error {
+func applyCRDsFromFile(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface, filePath string) error {
 	file, err := os.Open(filePath)
 	if err != nil {
-		return fmt.Errorf("open file %q: %v", filePath, err)
+		return fmt.Errorf("open file %q: %w", filePath, err)
 	}
 	defer file.Close()
 
@@ -132,7 +133,7 @@ func applyCRDsFromFile(ctx context.Context, client *clientset.Clientset, filePat
 			if err == io.EOF {
 				break
 			}
-			return fmt.Errorf("decode YAML: %v", err)
+			return fmt.Errorf("decode YAML: %w", err)
 		}
 		if crd.GetObjectKind().GroupVersionKind().Kind != "CustomResourceDefinition" {
 			log.Printf("Skipping non-CRD object %s", crd.GetName())
@@ -144,38 +145,40 @@ func applyCRDsFromFile(ctx context.Context, client *clientset.Clientset, filePat
 	// Apply each CRD separately.
 	for _, crd := range crdsToApply {
 		err := wait.ExponentialBackoffWithContext(ctx, retry.DefaultBackoff, func(context.Context) (bool, error) {
-			if err := applyCRD(ctx, client, crd); err != nil {
-				//nolint:nilerr
+			if err := applyCRD(ctx, crdClient, crd); err != nil {
+				log.Printf("Failed to apply CRD %s: %v", crd.Name, err)
 				return false, nil
 			}
 			return true, nil
 		})
 		if err != nil {
-			return fmt.Errorf("apply CRD %s: %v", crd.Name, err)
+			return fmt.Errorf("apply CRD %s: %w", crd.Name, err)
 		}
 	}
 	return nil
 }
 
 // applyCRD creates or updates the CRD.
-func applyCRD(ctx context.Context, client *clientset.Clientset, crd *apiextensionsv1.CustomResourceDefinition) error {
-	crdClient := client.ApiextensionsV1().CustomResourceDefinitions()
-
+func applyCRD(
+	ctx context.Context,
+	crdClient v1.CustomResourceDefinitionInterface,
+	crd *apiextensionsv1.CustomResourceDefinition,
+) error {
 	// Check if CRD already exists in cluster and create if not found.
 	curCRD, err := crdClient.Get(ctx, crd.Name, metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
 		log.Printf("Create CRD %s", crd.Name)
 		_, err = crdClient.Create(ctx, crd, metav1.CreateOptions{})
 		if err != nil {
-			return fmt.Errorf("create CRD %s: %v", crd.Name, err)
+			return fmt.Errorf("create CRD %s: %w", crd.Name, err)
 		}
 	} else {
 		log.Printf("Update CRD %s", crd.Name)
 		// Set resource version to update an existing CRD.
 		crd.SetResourceVersion(curCRD.GetResourceVersion())
 		_, err = crdClient.Update(ctx, crd, metav1.UpdateOptions{})
 		if err != nil {
-			return fmt.Errorf("update CRD %s: %v", crd.Name, err)
+			return fmt.Errorf("update CRD %s: %w", crd.Name, err)
 		}
 	}
 	return nil

diff --git a/cmd/apply-crds/main_test.go b/cmd/apply-crds/main_test.go
@@ -35,42 +35,42 @@ var _ = Describe("CRD Application", func() {
 	})
 
 	AfterEach(func() {
-		Expect(testClient.ApiextensionsV1().CustomResourceDefinitions().DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{})).NotTo(HaveOccurred())
+		Expect(testCRDClient.DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{})).NotTo(HaveOccurred())
 	})
 
 	Describe("applyCRDsFromFile", func() {
 		It("should apply CRDs multiple times from a valid YAML file", func() {
 			By("applying CRDs")
-			Expect(applyCRDsFromFile(ctx, testClient, "test-files/test-crds.yaml")).To(Succeed())
-			Expect(applyCRDsFromFile(ctx, testClient, "test-files/test-crds.yaml")).To(Succeed())
-			Expect(applyCRDsFromFile(ctx, testClient, "test-files/test-crds.yaml")).To(Succeed())
-			Expect(applyCRDsFromFile(ctx, testClient, "test-files/test-crds.yaml")).To(Succeed())
+			Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
+			Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
+			Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
+			Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
 
 			By("verifying CRDs are applied")
-			crds, err := testClient.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{})
+			crds, err := testCRDClient.List(ctx, metav1.ListOptions{})
 			Expect(err).NotTo(HaveOccurred())
 			Expect(crds.Items).To(HaveLen(2))
 		})
 
 		It("should update CRDs", func() {
 			By("applying CRDs")
-			Expect(applyCRDsFromFile(ctx, testClient, "test-files/test-crds.yaml")).To(Succeed())
+			Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/test-crds.yaml")).To(Succeed())
 
 			By("verifying CRDs do not have spec.foobar")
 			for _, crdName := range []string{"bars.example.com", "foos.example.com"} {
-				crd, err := testClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, crdName, metav1.GetOptions{})
+				crd, err := testCRDClient.Get(ctx, crdName, metav1.GetOptions{})
 				Expect(err).NotTo(HaveOccurred())
 				props := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties
 				Expect(props).To(HaveKey("spec"))
 				Expect(props["spec"].Properties).NotTo(HaveKey("foobar"))
 			}
 
 			By("updating CRDs")
-			Expect(applyCRDsFromFile(ctx, testClient, "test-files/updated-test-crds.yaml")).To(Succeed())
+			Expect(applyCRDsFromFile(ctx, testCRDClient, "test-files/updated-test-crds.yaml")).To(Succeed())
 
 			By("verifying CRDs are updated")
 			for _, crdName := range []string{"bars.example.com", "foos.example.com"} {
-				crd, err := testClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, crdName, metav1.GetOptions{})
+				crd, err := testCRDClient.Get(ctx, crdName, metav1.GetOptions{})
 				Expect(err).NotTo(HaveOccurred())
 				props := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties
 				Expect(props["spec"].Properties).To(HaveKey("foobar"))

diff --git a/cmd/apply-crds/suite_test.go b/cmd/apply-crds/suite_test.go
@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	v1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -29,8 +30,8 @@ import (
 )
 
 var (
-	testClient *clientset.Clientset
-	testEnv    *envtest.Environment
+	testCRDClient v1.CustomResourceDefinitionInterface
+	testEnv       *envtest.Environment
 )
 
 func TestApplyCrds(t *testing.T) {
@@ -49,9 +50,10 @@ var _ = BeforeSuite(func() {
 	Expect(cfg).NotTo(BeNil())
 
 	// create clientset with scheme
-	testClient, err = clientset.NewForConfig(cfg)
+	client, err := clientset.NewForConfig(cfg)
 	Expect(err).NotTo(HaveOccurred())
-	Expect(testClient).NotTo(BeNil())
+	Expect(client).NotTo(BeNil())
+	testCRDClient = client.ApiextensionsV1().CustomResourceDefinitions()
 
 	go func() {
 		defer GinkgoRecover()