Merge branch 'jt/ff-transactional-pool-disconnect' into 'master'

featureflag: Remove `TransactionalAlternatesDisconnect`

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6569

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>

5 files changed, 6 insertions, 33 deletions

diff --git a/internal/featureflag/ff_transactional_alternates_disconnect.go b/internal/featureflag/ff_transactional_alternates_disconnect.go
deleted file mode 100644
index be6b4e88a..000000000
--- a/internal/featureflag/ff_transactional_alternates_disconnect.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package featureflag
-
-// TransactionalAlternatesDisconnect enables transactions to be used when disconnecting an alternate
-// object database from a Git repository. If the transaction fails, the alternate disconnect is
-// rolled back.
-var TransactionalAlternatesDisconnect = NewFeatureFlag(
-	"transactional_alternates_disconnect",
-	"v16.4.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/5540",
-	true,
-)
diff --git a/internal/gitaly/service/objectpool/alternates_test.go b/internal/gitaly/service/objectpool/alternates_test.go
index 6c8e00260..3b2959e6f 100644
--- a/internal/gitaly/service/objectpool/alternates_test.go
+++ b/internal/gitaly/service/objectpool/alternates_test.go
@@ -1,12 +1,10 @@
 package objectpool
 
 import (
-	"context"
 	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/storage"
@@ -18,10 +16,8 @@ import (
 
 func TestDisconnectGitAlternates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.TransactionalAlternatesDisconnect).Run(t, testDisconnectGitAlternates)
-}
 
-func testDisconnectGitAlternates(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repoProto, repoPath, _, client := setup(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -60,10 +56,8 @@ func testDisconnectGitAlternates(t *testing.T, ctx context.Context) {
 
 func TestDisconnectGitAlternatesNoAlternates(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.TransactionalAlternatesDisconnect).Run(t, testDisconnectGitAlternatesNoAlternates)
-}
 
-func testDisconnectGitAlternatesNoAlternates(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, repoProto, repoPath, _, client := setup(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index 6b3d78910..26b7fc00a 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -48,10 +48,7 @@ support this as the separate attributes file is going to be replaced with readin
 attributes from HEAD.`)
 
 	t.Parallel()
-	testhelper.NewFeatureSets(
-		featureflag.InterceptReplicateRepository,
-		featureflag.TransactionalAlternatesDisconnect,
-	).Run(t, testReplicateRepository)
+	testhelper.NewFeatureSets(featureflag.InterceptReplicateRepository).Run(t, testReplicateRepository)
 }
 
 func testReplicateRepository(t *testing.T, ctx context.Context) {
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 47cd76a27..da590fe0a 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -39,19 +39,13 @@ type transactionsCondition func(context.Context) bool
 func transactionsEnabled(context.Context) bool  { return true }
 func transactionsDisabled(context.Context) bool { return false }
 
-func transactionsFlag(flag featureflag.FeatureFlag) transactionsCondition {
-	return func(ctx context.Context) bool {
-		return flag.IsEnabled(ctx)
-	}
-}
-
 // transactionRPCs contains the list of repository-scoped mutating calls which may take part in
 // transactions. An optional feature flag can be added to conditionally enable transactional
 // behaviour. If none is given, it's always enabled.
 var transactionRPCs = map[string]transactionsCondition{
 	"/gitaly.CleanupService/ApplyBfgObjectMapStream":         transactionsEnabled,
 	"/gitaly.ConflictsService/ResolveConflicts":              transactionsEnabled,
-	"/gitaly.ObjectPoolService/DisconnectGitAlternates":      transactionsFlag(featureflag.TransactionalAlternatesDisconnect),
+	"/gitaly.ObjectPoolService/DisconnectGitAlternates":      transactionsEnabled,
 	"/gitaly.ObjectPoolService/FetchIntoObjectPool":          transactionsEnabled,
 	"/gitaly.ObjectPoolService/LinkRepositoryToObjectPool":   transactionsEnabled,
 	"/gitaly.OperationService/UserApplyPatch":                transactionsEnabled,
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index 212b4ee1b..22032ab68 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -2051,10 +2051,9 @@ func TestStreamDirectorStorageScopeError(t *testing.T) {
 
 func TestDisabledTransactionsWithFeatureFlag(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.TransactionalAlternatesDisconnect).Run(t, testDisabledTransactionsWithFeatureFlag)
-}
 
-func testDisabledTransactionsWithFeatureFlag(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	for rpc, enabledFn := range transactionRPCs {
 		if enabledFn(ctx) {
 			require.True(t, shouldUseTransaction(ctx, rpc))