featureflag: Add accessor to access list of feature flag definitions

The list of available feature flags is currently a globally accessible
variable that can be changed at will by other packages. Let's change
this to instead use an accessor function `DefinedFlags()`: this forbids
any change to the feature flags and allows us to iterate on the way we
store flags.

7 files changed, 26 insertions, 18 deletions

diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go
index 3aa10036f..ca0e6e4cc 100644
--- a/internal/gitaly/service/smarthttp/inforefs_test.go
+++ b/internal/gitaly/service/smarthttp/inforefs_test.go
@@ -360,7 +360,7 @@ func TestCacheInfoRefsUploadPack(t *testing.T) {
 	// Praefect explicitly injects all unset feature flags, the key is thus differend depending
 	// on whether Praefect is in use or not. We thus manually inject all feature flags here such
 	// that they're forced to the same state.
-	for _, ff := range featureflag.All {
+	for _, ff := range featureflag.DefinedFlags() {
 		ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, ff, true)
 		ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, ff, true)
 	}
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 981e658e5..79233fed4 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -68,7 +68,7 @@ func TestPostReceivePack_successful(t *testing.T) {
 	// the context's feature flags we see here and the context's metadata as it would
 	// arrive on the proxied Gitaly. To fix this, we thus inject all feature flags
 	// explicitly here.
-	for _, ff := range featureflag.All {
+	for _, ff := range featureflag.DefinedFlags() {
 		ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, ff, true)
 		ctx = featureflag.IncomingCtxWithFeatureFlag(ctx, ff, true)
 	}
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 66fc1e596..fe8c1ecfb 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -120,7 +120,7 @@ func TestReceivePackPushSuccess(t *testing.T) {
 	// when deserializing the HooksPayload. By setting all flags to `true` explicitly, we both
 	// verify that gitaly-ssh picks up feature flags correctly and fix the test to behave the
 	// same with and without Praefect.
-	for _, featureFlag := range featureflag.All {
+	for _, featureFlag := range featureflag.DefinedFlags() {
 		ctx = featureflag.ContextWithFeatureFlag(ctx, featureFlag, true)
 	}
 
@@ -153,7 +153,7 @@ func TestReceivePackPushSuccess(t *testing.T) {
 	payload.Transaction = nil
 
 	expectedFeatureFlags := featureflag.Raw{}
-	for _, feature := range featureflag.All {
+	for _, feature := range featureflag.DefinedFlags() {
 		expectedFeatureFlags[feature.MetadataKey()] = "true"
 	}
 
diff --git a/internal/metadata/featureflag/featureflag.go b/internal/metadata/featureflag/featureflag.go
index 0644fa424..38504bd43 100644
--- a/internal/metadata/featureflag/featureflag.go
+++ b/internal/metadata/featureflag/featureflag.go
@@ -32,10 +32,19 @@ var (
 		[]string{"flag", "enabled"},
 	)
 
-	// All is the list of all registered feature flags.
-	All = []FeatureFlag{}
+	// flagsByName is the set of defined feature flags mapped by their respective name.
+	flagsByName = map[string]FeatureFlag{}
 )
 
+// DefinedFlags returns the set of feature flags that have been explicitly defined.
+func DefinedFlags() []FeatureFlag {
+	flags := make([]FeatureFlag, 0, len(flagsByName))
+	for _, flag := range flagsByName {
+		flags = append(flags, flag)
+	}
+	return flags
+}
+
 // FeatureFlag gates the implementation of new or changed functionality.
 type FeatureFlag struct {
 	// Name is the name of the feature flag.
@@ -54,7 +63,9 @@ func NewFeatureFlag(name, version, rolloutIssueURL string, onByDefault bool) Fea
 		Name:        name,
 		OnByDefault: onByDefault,
 	}
-	All = append(All, featureFlag)
+
+	flagsByName[name] = featureFlag
+
 	return featureFlag
 }
 
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 38e61c0dc..82223bfb7 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -619,7 +619,7 @@ func streamParametersContext(ctx context.Context) context.Context {
 		rawFeatureFlags = map[string]string{}
 	}
 
-	for _, ff := range featureflag.All {
+	for _, ff := range featureflag.DefinedFlags() {
 		if _, ok := rawFeatureFlags[ff.MetadataKey()]; !ok {
 			rawFeatureFlags[ff.MetadataKey()] = strconv.FormatBool(ff.OnByDefault)
 		}
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index 0b8201f33..2fff9925b 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -2699,7 +2699,7 @@ func TestStreamParametersContext(t *testing.T) {
 
 	expectedFlags := func(overrides ...expectedFlag) []expectedFlag {
 		flagValues := map[featureflag.FeatureFlag]bool{}
-		for _, flag := range featureflag.All {
+		for _, flag := range featureflag.DefinedFlags() {
 			flagValues[flag] = flag.OnByDefault
 		}
 		for _, override := range overrides {
diff --git a/internal/testhelper/featureset_test.go b/internal/testhelper/featureset_test.go
index ddbb8652c..f3311b7d2 100644
--- a/internal/testhelper/featureset_test.go
+++ b/internal/testhelper/featureset_test.go
@@ -171,15 +171,12 @@ func TestNewFeatureSetsWithRubyFlags(t *testing.T) {
 }
 
 func TestFeatureSets_Run(t *testing.T) {
-	// This test depends on feature flags being default-enabled in the test
-	// context, which requires those flags to exist in the ff.All slice. So
-	// let's just append them here so we do not need to use a "real"
-	// feature flag, as that would require constant change when we remove
-	// old feature flags.
-	defer func(old []ff.FeatureFlag) {
-		ff.All = old
-	}(ff.All)
-	ff.All = append(ff.All, featureFlagA, featureFlagB)
+	// Define two default-enabled feature flags. Note that with `NewFeatureFlag()`, we
+	// automatically add them to the list of defined feature flags. While this is stateful and
+	// would theoretically also impact other tests, we don't really need to mind that given
+	// that we use test-specific names for the flags here.
+	featureFlagA := ff.NewFeatureFlag("global_feature_flag_a", "", "", true)
+	featureFlagB := ff.NewFeatureFlag("global_feature_flag_b", "", "", true)
 
 	var featureFlags [][2]bool
 	NewFeatureSets(featureFlagB, featureFlagA).Run(t, func(t *testing.T, ctx context.Context) {