Merge branch 'pks-drop-reference-transactions-feature-flag' into 'master'

featureflag: Remove reference transactions feature flag

See merge request gitlab-org/gitaly!3575

8 files changed, 31 insertions, 91 deletions

diff --git a/doc/design_ha.md b/doc/design_ha.md
index ff95c5484..b2885a952 100644
--- a/doc/design_ha.md
+++ b/doc/design_ha.md
@@ -511,12 +511,8 @@ changes.
 
 ## Using Strong Consistency
 
-The current implementation of strong consistency via reference-transaction hook
-is enabled by default. You can use the following feature flags to change its
-behavior:
-
-- `gitaly_reference_transactions`: This feature flag is enabled by default. If
-  disabled, reference transactions will not be used.
+The current implementation uses strong consistency via reference-transaction
+hooks.
 
 In order to observe reference transactions, the following metrics can be used:
 
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index d9fb4c936..72958ad7c 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -15,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
@@ -214,12 +213,9 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 }
 
 func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulGitHooksForUserCreateBranchRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	branchName := "new-branch"
@@ -405,12 +401,9 @@ func TestFailedUserCreateBranchRequest(t *testing.T) {
 }
 
 func TestSuccessfulUserDeleteBranchRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserDeleteBranchRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserDeleteBranchRequest(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index bfa690bff..c8d9cc4c0 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -19,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
@@ -32,12 +30,9 @@ import (
 )
 
 func TestSuccessfulUserDeleteTagRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserDeleteTagRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	tagNameInput := "to-be-deleted-soon-tag"
@@ -157,12 +152,9 @@ end`, cfg.Git.BinPath)
 }
 
 func TestSuccessfulUserCreateTagRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserCreateTagRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -405,12 +397,9 @@ func TestUserCreateTagWithTransaction(t *testing.T) {
 }
 
 func TestSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	preReceiveHook := writeAssertObjectTypePreReceiveHook(t, cfg)
@@ -497,12 +486,9 @@ func testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *tes
 }
 
 func TestSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserCreateTagRequestWithParsedTargetRevision)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	gittest.Exec(t, cfg, "-C", repoPath, "branch", "heads/master", "master~1")
@@ -839,16 +825,10 @@ func TestUserCreateTagStableTagIDs(t *testing.T) {
 	}, response.Tag)
 }
 
-// TODO: Rename to TestUserDeleteTag_successfulDeletionOfPrefixedTag,
-// see
-// https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2839#note_458751929
-func TestUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testUserDeleteTagsuccessfulDeletionOfPrefixedTag)
-}
+func TestUserDeleteTagSuccessfulDeletionOfPrefixedTag(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
@@ -1052,12 +1032,9 @@ func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) {
 }
 
 func TestFailedUserDeleteTagDueToHooks(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testFailedUserDeleteTagDueToHooks)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	tagNameInput := "to-be-deleted-soon-tag"
@@ -1280,12 +1257,9 @@ func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) {
 }
 
 func TestTagHookOutput(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testTagHookOutput)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testTagHookOutput(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go
index dcf058edc..8b8fbcce3 100644
--- a/internal/gitaly/service/operations/update_branches_test.go
+++ b/internal/gitaly/service/operations/update_branches_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"crypto/sha1"
 	"fmt"
 	"testing"
@@ -10,7 +9,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -25,12 +23,6 @@ var (
 )
 
 func TestSuccessfulUserUpdateBranchRequest(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testSuccessfulUserUpdateBranchRequest)
-}
-
-func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index fc3c9c116..43cb6440b 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -517,12 +517,9 @@ func TestPostReceivePackToHooks(t *testing.T) {
 }
 
 func TestPostReceiveWithTransactionsViaPraefect(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, testPostReceiveWithTransactionsViaPraefect)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testPostReceiveWithTransactionsViaPraefect(t *testing.T, ctx context.Context) {
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 
 	testhelper.ConfigureGitalyHooksBin(t, cfg)
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 934056344..efd896dce 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -9,8 +9,6 @@ type FeatureFlag struct {
 // In order to support coverage of combined features usage all feature flags should be marked as enabled for the test.
 // NOTE: if you add a new feature flag please add it to the `All` list defined below.
 var (
-	// ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency
-	ReferenceTransactions = FeatureFlag{Name: "reference_transactions", OnByDefault: true}
 	// GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror
 	GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false}
 	// FetchInternalRemoteErrors makes FetchInternalRemote return actual errors instead of a boolean
@@ -26,7 +24,6 @@ var (
 
 // All includes all feature flags.
 var All = []FeatureFlag{
-	ReferenceTransactions,
 	GoUpdateRemoteMirror,
 	FetchInternalRemoteErrors,
 	TxConfig,
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 844a53e9a..4631b7165 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -159,10 +159,6 @@ func init() {
 }
 
 func shouldUseTransaction(ctx context.Context, method string) bool {
-	if !featureflag.IsEnabled(ctx, featureflag.ReferenceTransactions) {
-		return false
-	}
-
 	condition, ok := transactionRPCs[method]
 	if !ok {
 		return false
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index 016d1991f..e3a26d930 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -21,7 +21,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/client"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/commonerr"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
@@ -1361,19 +1360,15 @@ func TestStreamDirectorStorageScopeError(t *testing.T) {
 }
 
 func TestDisabledTransactionsWithFeatureFlag(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.ReferenceTransactions,
-	}).Run(t, func(t *testing.T, ctx context.Context) {
-		for rpc, enabledFn := range transactionRPCs {
-			if enabledFn(ctx) {
-				require.Equal(t,
-					featureflag.IsEnabled(ctx, featureflag.ReferenceTransactions),
-					shouldUseTransaction(ctx, rpc),
-				)
-				break
-			}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	for rpc, enabledFn := range transactionRPCs {
+		if enabledFn(ctx) {
+			require.True(t, shouldUseTransaction(ctx, rpc))
+			break
 		}
-	})
+	}
 }
 
 func requireScopeOperation(t *testing.T, registry *protoregistry.Registry, fullMethod string, scope protoregistry.Scope, op protoregistry.OpType) {