🌱 Part 5: Reduce number of variable sources. Final cleanup (#501)

* Refactor controller variable source Signed-off-by: Mikalai Radchuk <[email protected]> * Clean up the variable source code Signed-off-by: Mikalai Radchuk <[email protected]> --------- Signed-off-by: Mikalai Radchuk <[email protected]>
operator-framework · Nov 10, 2023 · 6222ad1 · 6222ad1
1 parent 7e321b4
commit 6222ad1
Show file tree

Hide file tree

Showing 13 changed files with 170 additions and 1,432 deletions.
diff --git a/internal/controllers/variable_source.go b/internal/controllers/variable_source.go
@@ -22,7 +22,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/operator-framework/deppy/pkg/deppy"
-	"github.com/operator-framework/deppy/pkg/deppy/input"
 	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
 
 	operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
@@ -64,21 +63,35 @@ func (v *VariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, er
 		return nil, err
 	}
 
-	// We are in process of getting rid of extra variable sources.
-	// See this for progress: https://github.com/operator-framework/operator-controller/issues/437
-	vs := variablesources.NestedVariableSource{
-		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewOperatorVariableSource(operatorList.Items, allBundles, inputVariableSource), nil
-		},
-		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewBundleDeploymentVariableSource(operatorList.Items, bundleDeploymentList.Items, allBundles, inputVariableSource), nil
-		},
-		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewBundlesAndDepsVariableSource(allBundles, inputVariableSource), nil
-		},
-		func(inputVariableSource input.VariableSource) (input.VariableSource, error) {
-			return variablesources.NewCRDUniquenessConstraintsVariableSource(inputVariableSource), nil
-		},
+	requiredPackages, err := variablesources.MakeRequiredPackageVariables(allBundles, operatorList.Items)
+	if err != nil {
+		return nil, err
+	}
+
+	installedPackages, err := variablesources.MakeInstalledPackageVariables(allBundles, operatorList.Items, bundleDeploymentList.Items)
+	if err != nil {
+		return nil, err
+	}
+
+	bundles, err := variablesources.MakeBundleVariables(allBundles, requiredPackages, installedPackages)
+	if err != nil {
+		return nil, err
+	}
+
+	bundleUniqueness := variablesources.MakeBundleUniquenessVariables(bundles)
+
+	result := []deppy.Variable{}
+	for _, v := range requiredPackages {
+		result = append(result, v)
+	}
+	for _, v := range installedPackages {
+		result = append(result, v)
+	}
+	for _, v := range bundles {
+		result = append(result, v)
+	}
+	for _, v := range bundleUniqueness {
+		result = append(result, v)
 	}
-	return vs.GetVariables(ctx)
+	return result, nil
 }
diff --git a/internal/controllers/variable_source_test.go b/internal/controllers/variable_source_test.go
@@ -0,0 +1,139 @@
+package controllers_test
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/rand"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/utils/pointer"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/operator-framework/deppy/pkg/deppy"
+	"github.com/operator-framework/deppy/pkg/deppy/constraint"
+	"github.com/operator-framework/deppy/pkg/deppy/input"
+	"github.com/operator-framework/operator-registry/alpha/declcfg"
+	"github.com/operator-framework/operator-registry/alpha/property"
+	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
+
+	operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
+	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
+	"github.com/operator-framework/operator-controller/internal/controllers"
+	olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables"
+	testutil "github.com/operator-framework/operator-controller/test/util"
+)
+
+func TestVariableSource(t *testing.T) {
+	sch := runtime.NewScheme()
+	utilruntime.Must(operatorsv1alpha1.AddToScheme(sch))
+	utilruntime.Must(rukpakv1alpha1.AddToScheme(sch))
+
+	stableChannel := catalogmetadata.Channel{Channel: declcfg.Channel{
+		Name: "stable",
+		Entries: []declcfg.ChannelEntry{
+			{
+				Name: "packageA.v2.0.0",
+			},
+		},
+	}}
+	bundleSet := map[string]*catalogmetadata.Bundle{
+		"packageA.v2.0.0": {
+			Bundle: declcfg.Bundle{
+				Name:    "packageA.v2.0.0",
+				Package: "packageA",
+				Image:   "foo.io/packageA/packageA:v2.0.0",
+				Properties: []property.Property{
+					{Type: property.TypePackage, Value: json.RawMessage(`{"packageName":"packageA","version":"2.0.0"}`)},
+				},
+			},
+			CatalogName: "fake-catalog",
+			InChannels:  []*catalogmetadata.Channel{&stableChannel},
+		},
+	}
+	allBundles := make([]*catalogmetadata.Bundle, 0, len(bundleSet))
+	for _, bundle := range bundleSet {
+		allBundles = append(allBundles, bundle)
+	}
+
+	pkgName := "packageA"
+	opName := fmt.Sprintf("operator-test-%s", rand.String(8))
+	operator := &operatorsv1alpha1.Operator{
+		ObjectMeta: metav1.ObjectMeta{Name: opName},
+		Spec: operatorsv1alpha1.OperatorSpec{
+			PackageName: pkgName,
+			Channel:     "stable",
+			Version:     "2.0.0",
+		},
+	}
+
+	bd := &rukpakv1alpha1.BundleDeployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: opName,
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion:         operatorsv1alpha1.GroupVersion.String(),
+					Kind:               "Operator",
+					Name:               operator.Name,
+					UID:                operator.UID,
+					Controller:         pointer.Bool(true),
+					BlockOwnerDeletion: pointer.Bool(true),
+				},
+			},
+		},
+		Spec: rukpakv1alpha1.BundleDeploymentSpec{
+			ProvisionerClassName: "core-rukpak-io-plain",
+			Template: &rukpakv1alpha1.BundleTemplate{
+				Spec: rukpakv1alpha1.BundleSpec{
+					ProvisionerClassName: "core-rukpak-io-registry",
+					Source: rukpakv1alpha1.BundleSource{
+						Type: rukpakv1alpha1.SourceTypeImage,
+						Image: &rukpakv1alpha1.ImageSource{
+							Ref: "foo.io/packageA/packageA:v2.0.0",
+						},
+					},
+				},
+			},
+		},
+	}
+	fakeClient := fake.NewClientBuilder().WithScheme(sch).WithObjects(operator, bd).Build()
+	fakeCatalogClient := testutil.NewFakeCatalogClient(allBundles)
+
+	vs := controllers.NewVariableSource(fakeClient, &fakeCatalogClient)
+
+	vars, err := vs.GetVariables(context.Background())
+	require.NoError(t, err)
+
+	expectedVars := []deppy.Variable{
+		olmvariables.NewRequiredPackageVariable("packageA", []*catalogmetadata.Bundle{
+			bundleSet["packageA.v2.0.0"],
+		}),
+		olmvariables.NewInstalledPackageVariable("packageA", []*catalogmetadata.Bundle{
+			bundleSet["packageA.v2.0.0"],
+		}),
+		olmvariables.NewBundleVariable(bundleSet["packageA.v2.0.0"], nil),
+		olmvariables.NewBundleUniquenessVariable(
+			"packageA package uniqueness",
+			deppy.Identifier("fake-catalog-packageA-packageA.v2.0.0"),
+		),
+	}
+	gocmpopts := []cmp.Option{
+		cmpopts.IgnoreUnexported(catalogmetadata.Bundle{}),
+		cmp.AllowUnexported(
+			olmvariables.RequiredPackageVariable{},
+			olmvariables.InstalledPackageVariable{},
+			olmvariables.BundleVariable{},
+			olmvariables.BundleUniquenessVariable{},
+			input.SimpleVariable{},
+			constraint.DependencyConstraint{},
+			constraint.AtMostConstraint{},
+		),
+	}
+	require.Empty(t, cmp.Diff(vars, expectedVars, gocmpopts...))
+}
diff --git a/internal/resolution/variablesources/bundle.go b/internal/resolution/variablesources/bundle.go
@@ -1,12 +1,10 @@
 package variablesources
 
 import (
-	"context"
 	"fmt"
 	"sort"
 
 	"github.com/operator-framework/deppy/pkg/deppy"
-	"github.com/operator-framework/deppy/pkg/deppy/input"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata"
@@ -15,53 +13,6 @@ import (
 	olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables"
 )
 
-var _ input.VariableSource = &BundlesAndDepsVariableSource{}
-
-type BundlesAndDepsVariableSource struct {
-	allBundles      []*catalogmetadata.Bundle
-	variableSources []input.VariableSource
-}
-
-func NewBundlesAndDepsVariableSource(allBundles []*catalogmetadata.Bundle, inputVariableSources ...input.VariableSource) *BundlesAndDepsVariableSource {
-	return &BundlesAndDepsVariableSource{
-		allBundles:      allBundles,
-		variableSources: inputVariableSources,
-	}
-}
-
-func (b *BundlesAndDepsVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) {
-	variables := []deppy.Variable{}
-
-	for _, variableSource := range b.variableSources {
-		inputVariables, err := variableSource.GetVariables(ctx)
-		if err != nil {
-			return nil, err
-		}
-		variables = append(variables, inputVariables...)
-	}
-
-	requiredPackages := []*olmvariables.RequiredPackageVariable{}
-	installedPackages := []*olmvariables.InstalledPackageVariable{}
-	for _, variable := range variables {
-		switch v := variable.(type) {
-		case *olmvariables.RequiredPackageVariable:
-			requiredPackages = append(requiredPackages, v)
-		case *olmvariables.InstalledPackageVariable:
-			installedPackages = append(installedPackages, v)
-		}
-	}
-
-	bundles, err := MakeBundleVariables(b.allBundles, requiredPackages, installedPackages)
-	if err != nil {
-		return nil, err
-	}
-
-	for _, v := range bundles {
-		variables = append(variables, v)
-	}
-	return variables, nil
-}
-
 func MakeBundleVariables(
 	allBundles []*catalogmetadata.Bundle,
 	requiredPackages []*olmvariables.RequiredPackageVariable,

diff --git a/internal/resolution/variablesources/bundle_deployment.go b/internal/resolution/variablesources/bundle_deployment.go