Merge branch 'jt-remove-hooks-transaction-ff' into 'master'

featureflag: Remove `tx_restore_custom_hooks`

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

Merged-by: Sami Hiltunen <shiltunen@gitlab.com>
Approved-by: Sami Hiltunen <shiltunen@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>

7 files changed, 18 insertions, 83 deletions

diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index 63342f5c2..6f6ddeeb0 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -19,7 +19,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/setup"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -298,11 +297,7 @@ func TestManager_Create_incremental(t *testing.T) {
 func TestManager_Restore(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks).
-		Run(t, testManagerRestore)
-}
-
-func testManagerRestore(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	testcfg.BuildGitalyHooks(t, cfg)
 
diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go
index 2362dd2ce..cc5675fd7 100644
--- a/internal/gitaly/service/repository/replicate.go
+++ b/internal/gitaly/service/repository/replicate.go
@@ -283,7 +283,7 @@ func (s *server) syncCustomHooks(ctx context.Context, in *gitalypb.ReplicateRepo
 		return request.GetData(), err
 	})
 
-	if err := s.setCustomHooksTransaction(ctx, reader, in.GetRepository()); err != nil {
+	if err := s.setCustomHooks(ctx, reader, in.GetRepository()); err != nil {
 		return fmt.Errorf("setting custom hooks: %w", err)
 	}
 
diff --git a/internal/gitaly/service/repository/set_custom_hooks.go b/internal/gitaly/service/repository/set_custom_hooks.go
index 0898eee43..3616b8136 100644
--- a/internal/gitaly/service/repository/set_custom_hooks.go
+++ b/internal/gitaly/service/repository/set_custom_hooks.go
@@ -16,7 +16,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/safe"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/tempdir"
@@ -54,7 +53,7 @@ func (s *server) SetCustomHooks(stream gitalypb.RepositoryService_SetCustomHooks
 	})
 
 	if err := s.setCustomHooks(ctx, reader, repo); err != nil {
-		return structerr.NewInternal("%w", err)
+		return structerr.NewInternal("setting custom hooks: %w", err)
 	}
 
 	return stream.SendAndClose(&gitalypb.SetCustomHooksResponse{})
@@ -88,36 +87,16 @@ func (s *server) RestoreCustomHooks(stream gitalypb.RepositoryService_RestoreCus
 	})
 
 	if err := s.setCustomHooks(ctx, reader, repo); err != nil {
-		return structerr.NewInternal("%w", err)
+		return structerr.NewInternal("setting custom hooks: %w", err)
 	}
 
 	return stream.SendAndClose(&gitalypb.RestoreCustomHooksResponse{})
 }
 
+// setCustomHooks transactionally and atomically sets custom hooks for a
+// repository. The provided reader should be a tarball containing the custom
+// hooks to be extracted to the specified Git repository.
 func (s *server) setCustomHooks(ctx context.Context, reader io.Reader, repo repository.GitRepo) error {
-	if featureflag.TransactionalRestoreCustomHooks.IsEnabled(ctx) {
-		if err := s.setCustomHooksTransaction(ctx, reader, repo); err != nil {
-			return fmt.Errorf("setting custom hooks: %w", err)
-		}
-
-		return nil
-	}
-
-	repoPath, err := s.locator.GetRepoPath(repo)
-	if err != nil {
-		return fmt.Errorf("getting repo path: %w", err)
-	}
-
-	if err := hook.ExtractHooks(ctx, reader, repoPath, false); err != nil {
-		return fmt.Errorf("extracting hooks: %w", err)
-	}
-
-	return nil
-}
-
-// setCustomHooksTransaction transactionally and atomically sets the provided
-// custom hooks for the specified repository.
-func (s *server) setCustomHooksTransaction(ctx context.Context, tar io.Reader, repo repository.GitRepo) error {
 	repoPath, err := s.locator.GetRepoPath(repo)
 	if err != nil {
 		return fmt.Errorf("getting repo path: %w", err)
@@ -157,7 +136,7 @@ func (s *server) setCustomHooksTransaction(ctx context.Context, tar io.Reader, r
 		}
 	}()
 
-	if err := hook.ExtractHooks(ctx, tar, tmpDir.Path(), false); err != nil {
+	if err := hook.ExtractHooks(ctx, reader, tmpDir.Path(), false); err != nil {
 		return fmt.Errorf("extracting hooks: %w", err)
 	}
 
diff --git a/internal/gitaly/service/repository/set_custom_hooks_test.go b/internal/gitaly/service/repository/set_custom_hooks_test.go
index c39d587ab..4e1b47cd3 100644
--- a/internal/gitaly/service/repository/set_custom_hooks_test.go
+++ b/internal/gitaly/service/repository/set_custom_hooks_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
@@ -32,12 +31,8 @@ import (
 
 func TestSetCustomHooksRequest_success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks).
-		Run(t, testSuccessfulSetCustomHooksRequest)
-}
 
-func testSuccessfulSetCustomHooksRequest(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc         string
@@ -136,27 +131,18 @@ func testSuccessfulSetCustomHooksRequest(t *testing.T, ctx context.Context) {
 			require.NoError(t, err)
 
 			require.FileExists(t, filepath.Join(repoPath, "custom_hooks", "pre-push.sample"))
-
-			if featureflag.TransactionalRestoreCustomHooks.IsEnabled(ctx) {
-				require.Equal(t, 2, len(txManager.Votes()))
-				assert.Equal(t, voting.Prepared, txManager.Votes()[0].Phase)
-				assert.Equal(t, expectedVote, txManager.Votes()[1].Vote)
-				assert.Equal(t, voting.Committed, txManager.Votes()[1].Phase)
-			} else {
-				require.Equal(t, 0, len(txManager.Votes()))
-			}
+			require.Equal(t, 2, len(txManager.Votes()))
+			assert.Equal(t, voting.Prepared, txManager.Votes()[0].Phase)
+			assert.Equal(t, expectedVote, txManager.Votes()[1].Vote)
+			assert.Equal(t, voting.Committed, txManager.Votes()[1].Phase)
 		})
 	}
 }
 
 func TestSetCustomHooks_failedValidation(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks).
-		Run(t, testFailedSetCustomHooksDueToValidations)
-}
 
-func testFailedSetCustomHooksDueToValidations(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc         string
@@ -202,12 +188,8 @@ func testFailedSetCustomHooksDueToValidations(t *testing.T, ctx context.Context)
 
 func TestSetCustomHooks_corruptTar(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks).
-		Run(t, testFailedSetCustomHooksDueToBadTar)
-}
 
-func testFailedSetCustomHooksDueToBadTar(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc         string
diff --git a/internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go b/internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go
deleted file mode 100644
index 6e92a1b77..000000000
--- a/internal/metadata/featureflag/ff_transactional_restore_custom_hooks.go
+++ /dev/null
@@ -1,10 +0,0 @@
-package featureflag
-
-// TransactionalRestoreCustomHooks will use transactional voting in the
-// RestoreCustomHooks RPC
-var TransactionalRestoreCustomHooks = NewFeatureFlag(
-	"tx_restore_custom_hooks",
-	"v15.0.0",
-	"https://gitlab.com/gitlab-org/gitaly/-/issues/4203",
-	true,
-)
diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 3a2c00497..44f21e82b 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -42,12 +42,6 @@ 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.
@@ -82,6 +76,8 @@ var transactionRPCs = map[string]transactionsCondition{
 	"/gitaly.RepositoryService/FetchSourceBranch":            transactionsEnabled,
 	"/gitaly.RepositoryService/RemoveRepository":             transactionsEnabled,
 	"/gitaly.RepositoryService/ReplicateRepository":          transactionsEnabled,
+	"/gitaly.RepositoryService/RestoreCustomHooks":           transactionsEnabled,
+	"/gitaly.RepositoryService/SetCustomHooks":               transactionsEnabled,
 	"/gitaly.RepositoryService/SetFullPath":                  transactionsEnabled,
 	"/gitaly.RepositoryService/WriteRef":                     transactionsEnabled,
 	"/gitaly.SSHService/SSHReceivePack":                      transactionsEnabled,
@@ -95,11 +91,6 @@ var transactionRPCs = map[string]transactionsCondition{
 	"/gitaly.ObjectPoolService/LinkRepositoryToObjectPool": transactionsDisabled,
 	"/gitaly.ObjectPoolService/ReduplicateRepository":      transactionsDisabled,
 	"/gitaly.RepositoryService/RenameRepository":           transactionsDisabled,
-
-	// The `RestoreCustomHooks` RPC can be make transactional by enabling the
-	// `TransactionalRestoreCustomHooks` feature flag.
-	"/gitaly.RepositoryService/RestoreCustomHooks": transactionsFlag(featureflag.TransactionalRestoreCustomHooks),
-	"/gitaly.RepositoryService/SetCustomHooks":     transactionsFlag(featureflag.TransactionalRestoreCustomHooks),
 }
 
 // forcePrimaryRoutingRPCs tracks RPCs which need to always get routed to the primary. This should
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index ae6d72e19..7b101c03c 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -1993,10 +1993,8 @@ func TestStreamDirectorStorageScopeError(t *testing.T) {
 func TestDisabledTransactionsWithFeatureFlag(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.TransactionalRestoreCustomHooks).Run(t, testDisabledTransactionsWithFeatureFlag)
-}
+	ctx := testhelper.Context(t)
 
-func testDisabledTransactionsWithFeatureFlag(t *testing.T, ctx context.Context) {
 	for rpc, enabledFn := range transactionRPCs {
 		if enabledFn(ctx) {
 			require.True(t, shouldUseTransaction(ctx, rpc))