fix empty action config client and update catalog creation unit test (#226)

Also adding some nil value checks for printing clusterCatalogs and clusterExtensions.

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

Author: ankitathomas

internal/cmd/internal/olmv1/catalog_create.go

@@ -13,7 +13,7 @@ import (
 
 // NewCatalogCreateCmd allows creating a new catalog
 func NewCatalogCreateCmd(cfg *action.Configuration) *cobra.Command {
-	i := v1action.NewCatalogCreate(cfg.Client)
+	i := v1action.NewCatalogCreate(cfg)
 	i.Logf = log.Printf
 
 	cmd := &cobra.Command{

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/pkg/v1/action/action_suite_test.go

@@ -12,43 +12,85 @@ 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"
 
 	olmv1 "github.com/operator-framework/operator-controller/api/v1"
+
+	"github.com/operator-framework/kubectl-operator/pkg/action"
+)
+
+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.go

@@ -7,16 +7,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	olmv1 "github.com/operator-framework/operator-controller/api/v1"
-)
 
-type createClient interface {
-	creator
-	deleter
-	getter
-}
+	"github.com/operator-framework/kubectl-operator/pkg/action"
+)
 
 type CatalogCreate struct {
-	client         createClient
+	config         *action.Configuration
 	CatalogName    string
 	ImageSourceRef string
 
@@ -29,28 +25,28 @@ type CatalogCreate struct {
 	Logf func(string, ...interface{})
 }
 
-func NewCatalogCreate(client createClient) *CatalogCreate {
+func NewCatalogCreate(config *action.Configuration) *CatalogCreate {
 	return &CatalogCreate{
-		client: client,
+		config: config,
 		Logf:   func(string, ...interface{}) {},
 	}
 }
 
 func (i *CatalogCreate) Run(ctx context.Context) error {
 	catalog := i.buildCatalog()
-	if err := i.client.Create(ctx, &catalog); err != nil {
+	if err := i.config.Client.Create(ctx, &catalog); err != nil {
 		return err
 	}
 
 	var err error
 	if i.Available {
-		err = waitUntilCatalogStatusCondition(ctx, i.client, &catalog, olmv1.TypeServing, metav1.ConditionTrue)
+		err = waitUntilCatalogStatusCondition(ctx, i.config.Client, &catalog, olmv1.TypeServing, metav1.ConditionTrue)
 	} else {
-		err = waitUntilCatalogStatusCondition(ctx, i.client, &catalog, olmv1.TypeServing, metav1.ConditionFalse)
+		err = waitUntilCatalogStatusCondition(ctx, i.config.Client, &catalog, olmv1.TypeServing, metav1.ConditionFalse)
 	}
 
 	if err != nil {
-		if cleanupErr := deleteWithTimeout(i.client, &catalog, i.CleanupTimeout); cleanupErr != nil {
+		if cleanupErr := deleteWithTimeout(i.config.Client, &catalog, i.CleanupTimeout); cleanupErr != nil {
 			i.Logf("cleaning up failed catalog: %v", cleanupErr)
 		}
 		return err

internal/pkg/v1/action/catalog_create_test.go

@@ -8,30 +8,21 @@ import (
 	. "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"
+	"github.com/operator-framework/kubectl-operator/pkg/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 +40,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(&action.Configuration{Client: testClient})
 		creator.Available = true
 		creator.CatalogName = expectedCatalog.Name
 		creator.ImageSourceRef = expectedCatalog.Spec.Source.Image.Ref
@@ -62,41 +54,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(&action.Configuration{Client: 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(&action.Configuration{Client: 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(&action.Configuration{Client: 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/interfaces.go

@@ -6,10 +6,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-type creator interface {
-	Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error
-}
-
 type deleter interface {
 	Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) 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())
 		}()