Merge pull request #1131 from hongkailiu/no-issues-simple-1

NO-JIRA: Refactor for readability

Author: openshift-merge-bot[bot]

lib/capability/capability.go

@@ -8,12 +8,6 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 )
 
-const (
-	CapabilityAnnotation = "capability.openshift.io/name"
-
-	DefaultCapabilitySet = configv1.ClusterVersionCapabilitySetCurrent
-)
-
 type ClusterCapabilities struct {
 	KnownCapabilities             map[configv1.ClusterVersionCapability]struct{}
 	EnabledCapabilities           map[configv1.ClusterVersionCapability]struct{}
@@ -34,18 +28,20 @@ func (caps capabilitiesSort) Len() int           { return len(caps) }
 func (caps capabilitiesSort) Swap(i, j int)      { caps[i], caps[j] = caps[j], caps[i] }
 func (caps capabilitiesSort) Less(i, j int) bool { return string(caps[i]) < string(caps[j]) }
 
-// SetCapabilities populates and returns cluster capabilities from ClusterVersion capabilities spec. This method also
-// ensures that no previously enabled capability is now disabled and returns any such implicitly enabled capabilities.
+// SetCapabilities populates and returns cluster capabilities from ClusterVersion's capabilities specification and a
+// collection of capabilities that are enabled including implicitly enabled.
+// It ensures that each capability in the collection is still enabled in the returned cluster capabilities
+// and recognizes all implicitly enabled ones.
 func SetCapabilities(config *configv1.ClusterVersion,
-	existingEnabled, alwaysEnabled map[configv1.ClusterVersionCapability]struct{}) ClusterCapabilities {
+	capabilities []configv1.ClusterVersionCapability) ClusterCapabilities {
 
-	var capabilities ClusterCapabilities
-	capabilities.KnownCapabilities = setKnownCapabilities()
+	var clusterCapabilities ClusterCapabilities
+	clusterCapabilities.KnownCapabilities = GetCapabilitiesAsMap(configv1.KnownClusterVersionCapabilities)
 
-	capabilities.EnabledCapabilities, capabilities.ImplicitlyEnabledCapabilities = setEnabledCapabilities(config.Spec.Capabilities,
-		existingEnabled, alwaysEnabled)
+	clusterCapabilities.EnabledCapabilities, clusterCapabilities.ImplicitlyEnabledCapabilities =
+		categorizeEnabledCapabilities(config.Spec.Capabilities, capabilities)
 
-	return capabilities
+	return clusterCapabilities
 }
 
 // GetCapabilitiesAsMap returns the slice of capabilities as a map with default values.
@@ -72,17 +68,6 @@ func SetFromImplicitlyEnabledCapabilities(implicitlyEnabled []configv1.ClusterVe
 	return capabilities
 }
 
-// GetKnownCapabilities returns all known capabilities as defined in ClusterVersion.
-func GetKnownCapabilities() []configv1.ClusterVersionCapability {
-	var known []configv1.ClusterVersionCapability
-
-	for _, v := range configv1.ClusterVersionCapabilitySets {
-		known = append(known, v...)
-	}
-	sort.Sort(capabilitiesSort(known))
-	return known
-}
-
 // GetCapabilitiesStatus populates and returns ClusterVersion capabilities status from given capabilities.
 func GetCapabilitiesStatus(capabilities ClusterCapabilities) configv1.ClusterVersionCapabilitiesStatus {
 	var status configv1.ClusterVersionCapabilitiesStatus
@@ -131,31 +116,13 @@ func Contains(caps []configv1.ClusterVersionCapability, capability configv1.Clus
 	return found
 }
 
-// setKnownCapabilities populates a map keyed by capability from all known capabilities as defined in ClusterVersion.
-func setKnownCapabilities() map[configv1.ClusterVersionCapability]struct{} {
-	known := make(map[configv1.ClusterVersionCapability]struct{})
-
-	for _, v := range configv1.ClusterVersionCapabilitySets {
-		for _, capability := range v {
-			if _, ok := known[capability]; ok {
-				continue
-			}
-			known[capability] = struct{}{}
-		}
-	}
-	return known
-}
-
-// setEnabledCapabilities populates a map keyed by capability from all enabled capabilities as defined in ClusterVersion.
-// DefaultCapabilitySet is used if a baseline capability set is not defined by ClusterVersion. A check is then made to
-// ensure that no previously enabled capability is now disabled and if any such capabilities are found each is enabled,
-// saved, and returned.
-// The required capabilities are added to the implicitly enabled.
-func setEnabledCapabilities(capabilitiesSpec *configv1.ClusterVersionCapabilitiesSpec,
-	priorEnabled, alwaysEnabled map[configv1.ClusterVersionCapability]struct{}) (map[configv1.ClusterVersionCapability]struct{},
+// categorizeEnabledCapabilities categorizes enabled capabilities by implicitness from cluster version's
+// capabilities specification and a collection of capabilities that are enabled including implicitly enabled.
+func categorizeEnabledCapabilities(capabilitiesSpec *configv1.ClusterVersionCapabilitiesSpec,
+	capabilities []configv1.ClusterVersionCapability) (map[configv1.ClusterVersionCapability]struct{},
 	[]configv1.ClusterVersionCapability) {
 
-	capSet := DefaultCapabilitySet
+	capSet := configv1.ClusterVersionCapabilitySetCurrent
 
 	if capabilitiesSpec != nil && len(capabilitiesSpec.BaselineCapabilitySet) > 0 {
 		capSet = capabilitiesSpec.BaselineCapabilitySet
@@ -171,13 +138,7 @@ func setEnabledCapabilities(capabilitiesSpec *configv1.ClusterVersionCapabilitie
 		}
 	}
 	var implicitlyEnabled []configv1.ClusterVersionCapability
-	for k := range priorEnabled {
-		if _, ok := enabled[k]; !ok {
-			implicitlyEnabled = append(implicitlyEnabled, k)
-			enabled[k] = struct{}{}
-		}
-	}
-	for k := range alwaysEnabled {
+	for _, k := range capabilities {
 		if _, ok := enabled[k]; !ok {
 			implicitlyEnabled = append(implicitlyEnabled, k)
 			enabled[k] = struct{}{}

lib/capability/capability_test.go

@@ -101,12 +101,12 @@ func TestSetCapabilities(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			caps := SetCapabilities(test.config, nil, nil)
+			caps := SetCapabilities(test.config, nil)
 			if test.config.Spec.Capabilities == nil || (test.config.Spec.Capabilities != nil &&
 				len(test.config.Spec.Capabilities.BaselineCapabilitySet) == 0) {
 
 				test.wantKnownKeys = configv1.KnownClusterVersionCapabilities
-				test.wantEnabledKeys = configv1.ClusterVersionCapabilitySets[DefaultCapabilitySet]
+				test.wantEnabledKeys = configv1.ClusterVersionCapabilitySets[configv1.ClusterVersionCapabilitySetCurrent]
 			}
 			if len(caps.KnownCapabilities) != len(test.wantKnownKeys) {
 				t.Errorf("Incorrect number of KnownCapabilities keys, wanted: %q. KnownCapabilities returned: %v", test.wantKnownKeys, caps.KnownCapabilities)
@@ -130,11 +130,10 @@ func TestSetCapabilities(t *testing.T) {
 
 func TestSetCapabilitiesWithImplicitlyEnabled(t *testing.T) {
 	tests := []struct {
-		name          string
-		config        *configv1.ClusterVersion
-		wantImplicit  []string
-		priorEnabled  map[configv1.ClusterVersionCapability]struct{}
-		alwaysEnabled map[configv1.ClusterVersionCapability]struct{}
+		name         string
+		config       *configv1.ClusterVersion
+		wantImplicit []string
+		priorEnabled []configv1.ClusterVersionCapability
 	}{
 		{name: "set baseline capabilities with additional",
 			config: &configv1.ClusterVersion{
@@ -144,36 +143,13 @@ func TestSetCapabilitiesWithImplicitlyEnabled(t *testing.T) {
 					},
 				},
 			},
-			priorEnabled: map[configv1.ClusterVersionCapability]struct{}{"cap1": {}, "cap2": {}, "cap3": {}},
+			priorEnabled: []configv1.ClusterVersionCapability{"cap1", "cap2", "cap3", "cap2"},
 			wantImplicit: []string{"cap1", "cap2", "cap3"},
 		},
-		{name: "set baseline capabilities with always enabled",
-			config: &configv1.ClusterVersion{
-				Spec: configv1.ClusterVersionSpec{
-					Capabilities: &configv1.ClusterVersionCapabilitiesSpec{
-						BaselineCapabilitySet: configv1.ClusterVersionCapabilitySetCurrent,
-					},
-				},
-			},
-			alwaysEnabled: map[configv1.ClusterVersionCapability]struct{}{"cap1": {}, "cap2": {}, "cap3": {}},
-			wantImplicit:  []string{"cap1", "cap2", "cap3"},
-		},
-		{name: "set baseline capabilities with additional and always enabled",
-			config: &configv1.ClusterVersion{
-				Spec: configv1.ClusterVersionSpec{
-					Capabilities: &configv1.ClusterVersionCapabilitiesSpec{
-						BaselineCapabilitySet: configv1.ClusterVersionCapabilitySetCurrent,
-					},
-				},
-			},
-			priorEnabled:  map[configv1.ClusterVersionCapability]struct{}{"cap1": {}, "cap2": {}, "cap3": {}},
-			alwaysEnabled: map[configv1.ClusterVersionCapability]struct{}{"cap1": {}, "cap2": {}, "cap4": {}},
-			wantImplicit:  []string{"cap1", "cap2", "cap3", "cap4"},
-		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			caps := SetCapabilities(test.config, test.priorEnabled, test.alwaysEnabled)
+			caps := SetCapabilities(test.config, test.priorEnabled)
 			if len(caps.ImplicitlyEnabledCapabilities) != len(test.wantImplicit) {
 				t.Errorf("Incorrect number of implicitly enabled keys, wanted: %q. ImplicitlyEnabledCapabilities returned: %v", test.wantImplicit, caps.ImplicitlyEnabledCapabilities)
 			}

pkg/cvo/cvo.go

@@ -37,7 +37,6 @@ import (
 	"github.com/openshift/library-go/pkg/verify/store/configmap"
 	"github.com/openshift/library-go/pkg/verify/store/sigstore"
 
-	"github.com/openshift/cluster-version-operator/lib/capability"
 	"github.com/openshift/cluster-version-operator/lib/resourcebuilder"
 	"github.com/openshift/cluster-version-operator/lib/validation"
 	"github.com/openshift/cluster-version-operator/pkg/clusterconditions"
@@ -288,7 +287,7 @@ func (optr *Operator) LoadInitialPayload(ctx context.Context, startingRequiredFe
 	}
 
 	update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.release.Image, optr.exclude, string(startingRequiredFeatureSet),
-		optr.clusterProfile, capability.GetKnownCapabilities())
+		optr.clusterProfile, configv1.KnownClusterVersionCapabilities)
 
 	if err != nil {
 		return nil, fmt.Errorf("the local release contents are invalid - no current version can be determined from disk: %v", err)

pkg/cvo/sync_worker.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
 	"math/rand"
 	"reflect"
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -483,7 +485,9 @@ func (w *SyncWorker) Update(ctx context.Context, generation int64, desired confi
 		return w.status.DeepCopy()
 	}
 
-	work.Capabilities = capability.SetCapabilities(config, priorCaps, capability.GetCapabilitiesAsMap(w.alwaysEnableCapabilities))
+	// ensureEnabledCapabilities includes both explicitly and implicitly enabled capabilities
+	ensureEnabledCapabilities := append(slices.Collect(maps.Keys(priorCaps)), w.alwaysEnableCapabilities...)
+	work.Capabilities = capability.SetCapabilities(config, ensureEnabledCapabilities)
 
 	versionEqual, overridesEqual, capabilitiesEqual :=
 		equalSyncWork(w.work, work, fmt.Sprintf("considering cluster version generation %d", generation))