fix empty action config client and update catalog creation unit test

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

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

@@ -7,46 +7,85 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/operator-framework/kubectl-operator/pkg/action"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	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 {
+type fakeClient 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 mockDeleter struct {
 	deleteErr    error
+	getErr       error
+	createCalled int
 	deleteCalled int
+	getCalled    int
+	transformers []objectTransformer
+	client.Client
 }
 
-func (md *mockDeleter) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
-	md.deleteCalled++
-	return md.deleteErr
+// meant to apply a function to anything matching the objectKey
+// when the fakeClient an action corresponding to verb.
+// transformer will not run if an error is already specified
+// for the verb in the fakeClient.
+// only implemented in Create for now
+type objectTransformer struct {
+	verb          string
+	objectKey     client.ObjectKey
+	transformFunc func(obj *client.Object)
 }
 
-type mockGetter struct {
-	getErr    error
-	getCalled int
-}
-
-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

@@ -4,6 +4,7 @@ import (
 	"context"
 	"time"
 
+	"github.com/operator-framework/kubectl-operator/pkg/action"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	olmv1 "github.com/operator-framework/operator-controller/api/v1"
@@ -29,9 +30,9 @@ type CatalogCreate struct {
 	Logf func(string, ...interface{})
 }
 
-func NewCatalogCreate(client createClient) *CatalogCreate {
+func NewCatalogCreate(cfg *action.Configuration) *CatalogCreate {
 	return &CatalogCreate{
-		client: client,
+		client: cfg.Client,
 		Logf:   func(string, ...interface{}) {},
 	}
 }

internal/pkg/v1/action/catalog_create_test.go

@@ -6,32 +6,22 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-
+	"github.com/operator-framework/kubectl-operator/pkg/action"
 	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 +39,10 @@ var _ = Describe("CatalogCreate", func() {
 
 	It("fails creating catalog", func() {
 		expectedErr := errors.New("create failed")
-		mockClient := &mockCreateClient{&mockCreator{createErr: expectedErr}, nil, nil, &expectedCatalog}
+		mockClient := fakeClient{createErr: expectedErr}
+		mockClient.Initialize()
 
-		creator := internalaction.NewCatalogCreate(mockClient)
+		creator := internalaction.NewCatalogCreate(&action.Configuration{Client: mockClient})
 		creator.Available = true
 		creator.CatalogName = expectedCatalog.Name
 		creator.ImageSourceRef = expectedCatalog.Spec.Source.Image.Ref
@@ -64,17 +55,22 @@ var _ = Describe("CatalogCreate", func() {
 		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)
+		//// 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
+		//actualCatalog := &olmv1catalogd.ClusterCatalog{}
+		//fmt.Printf("%+v, %+v", actualCatalog, expectedCatalog)
+		//Expect(mockClient.Get(context.TODO(), types.NamespacedName{Name: catalogName}, actualCatalog)).To(Succeed())
+		//Expect(actualCatalog).To(Equal(expectedCatalog))
 	})
 
 	It("fails waiting for created catalog status, successfully cleans up", func() {
 		expectedErr := errors.New("get failed")
-		mockClient := &mockCreateClient{&mockCreator{}, &mockGetter{getErr: expectedErr}, &mockDeleter{}, nil}
+		mockClient := fakeClient{getErr: expectedErr}
+		Expect(mockClient.Initialize()).To(Succeed())
 
-		creator := internalaction.NewCatalogCreate(mockClient)
+		creator := internalaction.NewCatalogCreate(&action.Configuration{Client: mockClient})
+		creator.CatalogName = expectedCatalog.Name
 		err := creator.Run(context.TODO())
 
 		Expect(err).NotTo(BeNil())
@@ -87,9 +83,12 @@ var _ = Describe("CatalogCreate", func() {
 	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}
+		mockClient := fakeClient{deleteErr: deleteErr, getErr: getErr}
+		Expect(mockClient.Initialize()).To(Succeed())
 
-		creator := internalaction.NewCatalogCreate(mockClient)
+		creator := internalaction.NewCatalogCreate(&action.Configuration{Client: mockClient})
+		// fakeClient requires at least the catalogName to be set to run
+		creator.CatalogName = expectedCatalog.Name
 		err := creator.Run(context.TODO())
 
 		Expect(err).NotTo(BeNil())
@@ -98,6 +97,42 @@ var _ = Describe("CatalogCreate", func() {
 		Expect(mockClient.getCalled).To(Equal(1))
 		Expect(mockClient.deleteCalled).To(Equal(1))
 	})
+	It("succeeds creating catalog", func() {
+		mockClient := 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(mockClient.Initialize()).To(Succeed())
+
+		creator := internalaction.NewCatalogCreate(&action.Configuration{Client: mockClient})
+		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(mockClient.createCalled).To(Equal(1))
+
+		actualCatalog := &olmv1.ClusterCatalog{TypeMeta: metav1.TypeMeta{Kind: "ClusterCatalog", APIVersion: "olm.operatorframework.io/v1"}}
+		mockClient.Client.Get(context.TODO(), types.NamespacedName{Name: catalogName}, actualCatalog)
+		validateCreateCatalog(actualCatalog, &expectedCatalog)
+	})
 })
 
 func validateCreateCatalog(actual, expected *olmv1.ClusterCatalog) {