fix empty action config client and update catalog creation unit test

Signed-off-by: Ankita Thomas <[email protected]>

Author: ankitathomas

internal/cmd/internal/olmv1/printing.go

@@ -21,11 +21,16 @@ func printFormattedOperators(extensions ...olmv1.ClusterExtension) {
 
 	sortOperators(extensions)
 	for _, ext := range extensions {
+		var bundleName, bundleVersion string
+		if ext.Status.Install != nil {
+			bundleName = ext.Status.Install.Bundle.Name
+			bundleVersion = ext.Status.Install.Bundle.Version
+		}
 		age := time.Since(ext.CreationTimestamp.Time)
 		_, _ = fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n",
 			ext.Name,
-			ext.Status.Install.Bundle.Name,
-			ext.Status.Install.Bundle.Version,
+			bundleName,
+			bundleVersion,
 			ext.Spec.Source.SourceType,
 			status(ext.Status.Conditions, olmv1.TypeInstalled),
 			status(ext.Status.Conditions, olmv1.TypeProgressing),
@@ -41,13 +46,16 @@ func printFormattedCatalogs(catalogs ...olmv1.ClusterCatalog) {
 
 	sortCatalogs(catalogs)
 	for _, cat := range catalogs {
+		var lastUnpacked string
+		if cat.Status.LastUnpacked != nil {
+			duration.HumanDuration(time.Since(cat.Status.LastUnpacked.Time))
+		}
 		age := time.Since(cat.CreationTimestamp.Time)
-		lastUnpacked := time.Since(cat.Status.LastUnpacked.Time)
 		_, _ = fmt.Fprintf(tw, "%s\t%s\t%d\t%s\t%s\t%s\n",
 			cat.Name,
 			string(cat.Spec.AvailabilityMode),
 			cat.Spec.Priority,
-			duration.HumanDuration(lastUnpacked),
+			lastUnpacked,
 			status(cat.Status.Conditions, olmv1.TypeServing),
 			duration.HumanDuration(age),
 		)
@@ -59,6 +67,9 @@ func printFormattedCatalogs(catalogs ...olmv1.ClusterCatalog) {
 // name (asc), version (desc)
 func sortOperators(extensions []olmv1.ClusterExtension) {
 	slices.SortFunc(extensions, func(a, b olmv1.ClusterExtension) int {
+		if a.Status.Install == nil || b.Status.Install == nil {
+			return cmp.Compare(a.Name, b.Name)
+		}
 		return cmp.Or(
 			cmp.Compare(a.Name, b.Name),
 			-semver.MustParse(a.Status.Install.Bundle.Version).Compare(semver.MustParse(b.Status.Install.Bundle.Version)),

internal/cmd/root.go

@@ -11,11 +11,16 @@ import (
 )
 
 func Execute() {
-	if err := newCmd().Execute(); err != nil {
+	cmd, err := newCmd()
+	if err != nil {
+		log.Fatal(err)
+	}
+
+	if err = cmd.Execute(); err != nil {
 		log.Fatal(err)
 	}
 }
-func newCmd() *cobra.Command {
+func newCmd() (*cobra.Command, error) {
 	cmd := &cobra.Command{
 		Use:   "operator",
 		Short: "Manage operators in a cluster from the command line",
@@ -36,14 +41,15 @@ operators from the installed catalogs.`,
 	flags := cmd.PersistentFlags()
 	cfg.BindFlags(flags)
 	flags.DurationVar(&timeout, "timeout", 1*time.Minute, "The amount of time to wait before giving up on an operation.")
-
-	cmd.PersistentPreRunE = func(cmd *cobra.Command, _ []string) error {
+	err := cfg.Load()
+	if err != nil {
+		return nil, err
+	}
+	cmd.PersistentPreRun = func(cmd *cobra.Command, _ []string) {
 		var ctx context.Context
 		ctx, cancel = context.WithTimeout(cmd.Context(), timeout)
 
 		cmd.SetContext(ctx)
-
-		return cfg.Load()
 	}
 	cmd.PersistentPostRun = func(command *cobra.Command, _ []string) {
 		cancel()
@@ -62,5 +68,5 @@ operators from the installed catalogs.`,
 		newVersionCmd(),
 	)
 
-	return cmd
+	return cmd, nil
 }

internal/pkg/v1/action/action_suite_test.go

@@ -12,43 +12,84 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
+	"github.com/operator-framework/kubectl-operator/pkg/action"
 	olmv1 "github.com/operator-framework/operator-controller/api/v1"
 )
 
+const (
+	verbCreate = "create"
+)
+
 func TestCommand(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Internal v1 action Suite")
 }
 
-type mockCreator struct {
-	createErr    error
-	createCalled int
-}
-
-func (mc *mockCreator) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
-	mc.createCalled++
-	return mc.createErr
-}
+type fakeClient struct {
+	// Expected errors for create/delete/get.
+	createErr error
+	deleteErr error
+	getErr    error
 
-type mockDeleter struct {
-	deleteErr    error
+	// counters for number of create/delete/get calls seen.
+	createCalled int
 	deleteCalled int
-}
+	getCalled    int
 
-func (md *mockDeleter) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
-	md.deleteCalled++
-	return md.deleteErr
+	// transformer functions for applying changes to an object
+	// matching the objectKey prior to an operation of the
+	// type `verb` (get/create/delete), where the operation is
+	// not set to error fail with a corresponding error (getErr/createErr/deleteErr).
+	transformers []objectTransformer
+	client.Client
 }
 
-type mockGetter struct {
-	getErr    error
-	getCalled int
+type objectTransformer struct {
+	verb          string
+	objectKey     client.ObjectKey
+	transformFunc func(obj *client.Object)
 }
 
-func (mg *mockGetter) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
-	mg.getCalled++
-	return mg.getErr
+func (c *fakeClient) Initialize() error {
+	scheme, err := action.NewScheme()
+	if err != nil {
+		return err
+	}
+	clientBuilder := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{
+		Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
+			c.createCalled++
+			if c.createErr != nil {
+				return c.createErr
+			}
+			objKey := types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}
+			for _, t := range c.transformers {
+				if t.verb == verbCreate && objKey == t.objectKey && t.transformFunc != nil {
+					t.transformFunc(&obj)
+				}
+			}
+			// make sure to plumb request through to underlying client
+			return client.Create(ctx, obj, opts...)
+		},
+		Delete: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.DeleteOption) error {
+			c.deleteCalled++
+			if c.deleteErr != nil {
+				return c.deleteErr
+			}
+			return client.Delete(ctx, obj, opts...)
+		},
+		Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
+			c.getCalled++
+			if c.getErr != nil {
+				return c.getErr
+			}
+			return client.Get(ctx, key, obj, opts...)
+		},
+	}).WithScheme(scheme)
+	c.Client = clientBuilder.Build()
+	return nil
 }
 
 func setupTestCatalogs(n int) []client.Object {

internal/pkg/v1/action/catalog_create_test.go

@@ -6,32 +6,21 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	olmv1 "github.com/operator-framework/operator-controller/api/v1"
 
 	internalaction "github.com/operator-framework/kubectl-operator/internal/pkg/v1/action"
 )
 
-type mockCreateClient struct {
-	*mockCreator
-	*mockGetter
-	*mockDeleter
-	createCatalog *olmv1.ClusterCatalog
-}
-
-func (mcc *mockCreateClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
-	mcc.createCatalog = obj.(*olmv1.ClusterCatalog)
-	return mcc.mockCreator.Create(ctx, obj, opts...)
-}
-
 var _ = Describe("CatalogCreate", func() {
+	catalogName := "testcatalog"
 	pollInterval := 20
 	expectedCatalog := olmv1.ClusterCatalog{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:   "testcatalog",
+			Name:   catalogName,
 			Labels: map[string]string{"a": "b"},
 		},
 		Spec: olmv1.ClusterCatalogSpec{
@@ -49,9 +38,10 @@ var _ = Describe("CatalogCreate", func() {
 
 	It("fails creating catalog", func() {
 		expectedErr := errors.New("create failed")
-		mockClient := &mockCreateClient{&mockCreator{createErr: expectedErr}, nil, nil, &expectedCatalog}
+		testClient := fakeClient{createErr: expectedErr}
+		Expect(testClient.Initialize()).To(Succeed())
 
-		creator := internalaction.NewCatalogCreate(mockClient)
+		creator := internalaction.NewCatalogCreate(testClient)
 		creator.Available = true
 		creator.CatalogName = expectedCatalog.Name
 		creator.ImageSourceRef = expectedCatalog.Spec.Source.Image.Ref
@@ -62,41 +52,78 @@ var _ = Describe("CatalogCreate", func() {
 
 		Expect(err).NotTo(BeNil())
 		Expect(err).To(MatchError(expectedErr))
-		Expect(mockClient.createCalled).To(Equal(1))
-
-		// there is no way of testing a happy path in unit tests because we have no way to
-		// set/mock the catalog status condition we're waiting for in waitUntilCatalogStatusCondition
-		// but we can still at least verify that CR would have been created with expected attribute values
-		validateCreateCatalog(mockClient.createCatalog, &expectedCatalog)
+		Expect(testClient.createCalled).To(Equal(1))
 	})
 
 	It("fails waiting for created catalog status, successfully cleans up", func() {
 		expectedErr := errors.New("get failed")
-		mockClient := &mockCreateClient{&mockCreator{}, &mockGetter{getErr: expectedErr}, &mockDeleter{}, nil}
+		testClient := fakeClient{getErr: expectedErr}
+		Expect(testClient.Initialize()).To(Succeed())
 
-		creator := internalaction.NewCatalogCreate(mockClient)
+		creator := internalaction.NewCatalogCreate(testClient)
+		// fakeClient requires at least the catalogName to be set to run
+		creator.CatalogName = expectedCatalog.Name
 		err := creator.Run(context.TODO())
 
 		Expect(err).NotTo(BeNil())
 		Expect(err).To(MatchError(expectedErr))
-		Expect(mockClient.createCalled).To(Equal(1))
-		Expect(mockClient.getCalled).To(Equal(1))
-		Expect(mockClient.deleteCalled).To(Equal(1))
+		Expect(testClient.createCalled).To(Equal(1))
+		Expect(testClient.getCalled).To(Equal(1))
+		Expect(testClient.deleteCalled).To(Equal(1))
 	})
 
 	It("fails waiting for created catalog status, fails clean up", func() {
 		getErr := errors.New("get failed")
 		deleteErr := errors.New("delete failed")
-		mockClient := &mockCreateClient{&mockCreator{}, &mockGetter{getErr: getErr}, &mockDeleter{deleteErr: deleteErr}, nil}
+		testClient := fakeClient{deleteErr: deleteErr, getErr: getErr}
+		Expect(testClient.Initialize()).To(Succeed())
 
-		creator := internalaction.NewCatalogCreate(mockClient)
+		creator := internalaction.NewCatalogCreate(testClient)
+		// fakeClient requires at least the catalogName to be set to run
+		creator.CatalogName = expectedCatalog.Name
 		err := creator.Run(context.TODO())
 
 		Expect(err).NotTo(BeNil())
 		Expect(err).To(MatchError(getErr))
-		Expect(mockClient.createCalled).To(Equal(1))
-		Expect(mockClient.getCalled).To(Equal(1))
-		Expect(mockClient.deleteCalled).To(Equal(1))
+		Expect(testClient.createCalled).To(Equal(1))
+		Expect(testClient.getCalled).To(Equal(1))
+		Expect(testClient.deleteCalled).To(Equal(1))
+	})
+	It("succeeds creating catalog", func() {
+		testClient := fakeClient{
+			transformers: []objectTransformer{
+				{
+					verb:      verbCreate,
+					objectKey: types.NamespacedName{Name: catalogName},
+					transformFunc: func(obj *client.Object) {
+						if obj == nil {
+							return
+						}
+						catalogObj, ok := (*obj).(*olmv1.ClusterCatalog)
+						if !ok {
+							return
+						}
+						catalogObj.Status.Conditions = []metav1.Condition{{Type: olmv1.TypeServing, Status: metav1.ConditionTrue}}
+					},
+				},
+			},
+		}
+		Expect(testClient.Initialize()).To(Succeed())
+
+		creator := internalaction.NewCatalogCreate(testClient)
+		creator.Available = true
+		creator.CatalogName = expectedCatalog.Name
+		creator.ImageSourceRef = expectedCatalog.Spec.Source.Image.Ref
+		creator.Priority = expectedCatalog.Spec.Priority
+		creator.Labels = expectedCatalog.Labels
+		creator.PollIntervalMinutes = *expectedCatalog.Spec.Source.Image.PollIntervalMinutes
+		Expect(creator.Run(context.TODO())).To(Succeed())
+
+		Expect(testClient.createCalled).To(Equal(1))
+
+		actualCatalog := &olmv1.ClusterCatalog{TypeMeta: metav1.TypeMeta{Kind: "ClusterCatalog", APIVersion: "olm.operatorframework.io/v1"}}
+		Expect(testClient.Client.Get(context.TODO(), types.NamespacedName{Name: catalogName}, actualCatalog)).To(Succeed())
+		validateCreateCatalog(actualCatalog, &expectedCatalog)
 	})
 })
 

internal/pkg/v1/action/helpers.go

@@ -80,9 +80,8 @@ func deleteWithTimeout(cl deleter, obj client.Object, timeout time.Duration) err
 	return nil
 }
 
-func waitForDeletion(ctx context.Context, cl client.Client, objs ...client.Object) error {
+func waitForDeletion(ctx context.Context, cl getter, objs ...client.Object) error {
 	for _, obj := range objs {
-		obj := obj
 		lowerKind := strings.ToLower(obj.GetObjectKind().GroupVersionKind().Kind)
 		key := objectKeyForObject(obj)
 		if err := wait.PollUntilContextCancel(ctx, pollInterval, true, func(conditionCtx context.Context) (bool, error) {

internal/pkg/v1/action/operator_update_test.go

@@ -145,8 +145,9 @@ var _ = Describe("OperatorUpdate", func() {
 		cfg := setupEnv(testExt, buildExtension("test2"), buildExtension("test3"))
 
 		go func() {
-			Eventually(updateOperatorConditionStatus("test", cfg.Client, olmv1.TypeInstalled, metav1.ConditionTrue)).
-				WithTimeout(5 * time.Second).WithPolling(200 * time.Second).
+			Eventually(updateOperatorConditionStatus).
+				WithArguments("test", cfg.Client, olmv1.TypeInstalled, metav1.ConditionTrue).
+				WithTimeout(5 * time.Second).WithPolling(200 * time.Millisecond).
 				Should(Succeed())
 		}()