Merge branch 'avar/nuke-ruby-user-create-tag-and-branch' into 'master'

Remove on-by-default go_user_delete_{branch,tag} feature flags

See merge request gitlab-org/gitaly!3033

6 files changed, 41 insertions, 114 deletions

diff --git a/changelogs/unreleased/avar-nuke-ruby-user-create-tag-and-branch.yml b/changelogs/unreleased/avar-nuke-ruby-user-create-tag-and-branch.yml
new file mode 100644
index 000000000..5f73b80c1
--- /dev/null
+++ b/changelogs/unreleased/avar-nuke-ruby-user-create-tag-and-branch.yml
@@ -0,0 +1,5 @@
+---
+title: Remove Ruby code for on-by-default go_user_delete_{branch,tag} feature flags
+merge_request: 3033
+author:
+type: changed
diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go
index a7cac180d..b9697c09d 100644
--- a/internal/gitaly/service/operations/branches.go
+++ b/internal/gitaly/service/operations/branches.go
@@ -16,12 +16,6 @@ import (
 )
 
 func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateBranchRequest) (*gitalypb.UserCreateBranchResponse, error) {
-	if featureflag.IsDisabled(ctx, featureflag.GoUserCreateBranch) {
-		return s.userCreateBranchRuby(ctx, req)
-	}
-
-	// Implement UserCreateBranch in Go
-
 	if len(req.BranchName) == 0 {
 		return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty branch name)")
 	}
@@ -78,20 +72,6 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB
 	}, nil
 }
 
-func (s *Server) userCreateBranchRuby(ctx context.Context, req *gitalypb.UserCreateBranchRequest) (*gitalypb.UserCreateBranchResponse, error) {
-	client, err := s.ruby.OperationServiceClient(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository())
-	if err != nil {
-		return nil, err
-	}
-
-	return client.UserCreateBranch(clientCtx, req)
-}
-
 func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) {
 	if featureflag.IsDisabled(ctx, featureflag.GoUserUpdateBranch) {
 		return s.userUpdateBranchRuby(ctx, req)
diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go
index 7d3917136..bc48e2f6b 100644
--- a/internal/gitaly/service/operations/branches_test.go
+++ b/internal/gitaly/service/operations/branches_test.go
@@ -35,10 +35,9 @@ func (s *testTransactionServer) VoteTransaction(ctx context.Context, in *gitalyp
 }
 
 func TestSuccessfulCreateBranchRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateBranch, testSuccessfulCreateBranchRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulCreateBranchRequest(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -217,7 +216,6 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 func TestSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
 		featureflag.ReferenceTransactions,
-		featureflag.GoUserCreateBranch,
 	}).Run(t, testSuccessfulGitHooksForUserCreateBranchRequest)
 }
 
@@ -256,15 +254,10 @@ func testSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T, ctx context.
 	}
 }
 
-func TestFailedUserCreateBranchDueToHooks(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateBranch, testFailedUserCreateBranchDueToHooks)
-}
-
 func TestSuccessfulCreateBranchRequestWithStartPointRefPrefix(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateBranch, testSuccessfulCreateBranchRequestWithStartPointRefPrefix)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulCreateBranchRequestWithStartPointRefPrefix(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -340,7 +333,10 @@ func testSuccessfulCreateBranchRequestWithStartPointRefPrefix(t *testing.T, ctx
 	}
 }
 
-func testFailedUserCreateBranchDueToHooks(t *testing.T, ctx context.Context) {
+func TestFailedUserCreateBranchDueToHooks(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -371,10 +367,9 @@ func testFailedUserCreateBranchDueToHooks(t *testing.T, ctx context.Context) {
 }
 
 func TestFailedUserCreateBranchRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateBranch, testFailedUserCreateBranchRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserCreateBranchRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -689,10 +684,9 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) {
 }
 
 func TestBranchHookOutput(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateBranch, testBranchHookOutput)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testBranchHookOutput(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index a3185b2cf..772900f02 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -12,9 +12,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -54,28 +52,7 @@ func (s *Server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagR
 	return &gitalypb.UserDeleteTagResponse{}, nil
 }
 
-func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) {
-	if featureflag.IsDisabled(ctx, featureflag.GoUserCreateTag) {
-		return s.userCreateTagRuby(ctx, req)
-	}
-	return s.userCreateTagGo(ctx, req)
-}
-
-func (s *Server) userCreateTagRuby(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) {
-	client, err := s.ruby.OperationServiceClient(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository())
-	if err != nil {
-		return nil, err
-	}
-
-	return client.UserCreateTag(clientCtx, req)
-}
-
-func validateUserCreateTagGo(req *gitalypb.UserCreateTagRequest) error {
+func validateUserCreateTag(req *gitalypb.UserCreateTagRequest) error {
 	// Emulate validations done by Ruby. A lot of these (e.g. the
 	// upper-case error messages) can be simplified once we're not
 	// doing bug-for-bug Ruby emulation anymore)
@@ -107,9 +84,9 @@ func validateUserCreateTagGo(req *gitalypb.UserCreateTagRequest) error {
 	return nil
 }
 
-func (s *Server) userCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) {
+func (s *Server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) {
 	// Validate the request
-	if err := validateUserCreateTagGo(req); err != nil {
+	if err := validateUserCreateTag(req); err != nil {
 		return nil, err
 	}
 
diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go
index 8e879667e..a3a45d62a 100644
--- a/internal/gitaly/service/operations/tags_test.go
+++ b/internal/gitaly/service/operations/tags_test.go
@@ -172,7 +172,6 @@ end`, config.Config.Git.BinPath)
 func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
 		featureflag.ReferenceTransactions,
-		featureflag.GoUserCreateTag,
 	}).Run(t, testSuccessfulUserCreateTagRequest)
 }
 
@@ -276,13 +275,10 @@ func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) {
 	}
 }
 
-func TestUserCreateTag_withTransaction(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoUserCreateTag,
-	}).Run(t, testUserCreateTagWithTransaction)
-}
+func TestUserCreateTagWithTransaction(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUserCreateTagWithTransaction(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -422,13 +418,7 @@ func testUserCreateTagWithTransaction(t *testing.T, ctx context.Context) {
 				testhelper.AssertPathNotExists(t, hooksOutputPath)
 			}
 
-			// The Ruby implementation only calls the
-			// reference-transaction hook once.
-			if featureflag.IsEnabled(ctx, featureflag.GoUserCreateTag) {
-				require.Equal(t, 2, transactionServer.called)
-			} else {
-				require.Equal(t, 1, transactionServer.called)
-			}
+			require.Equal(t, 2, transactionServer.called)
 			transactionServer.called = 0
 		})
 	}
@@ -437,7 +427,6 @@ func testUserCreateTagWithTransaction(t *testing.T, ctx context.Context) {
 func TestSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
 		featureflag.ReferenceTransactions,
-		featureflag.GoUserCreateTag,
 	}).Run(t, testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation)
 }
 
@@ -540,7 +529,6 @@ func testSuccessfulUserCreateTagRequestAnnotatedLightweightDisambiguation(t *tes
 func TestSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
 		featureflag.ReferenceTransactions,
-		featureflag.GoUserCreateTag,
 	}).Run(t, testSuccessfulUserCreateTagRequestWithParsedTargetRevision)
 }
 
@@ -628,10 +616,9 @@ func testSuccessfulUserCreateTagRequestWithParsedTargetRevision(t *testing.T, ct
 }
 
 func TestSuccessfulUserCreateTagRequestToNonCommit(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testSuccessfulUserCreateTagRequestToNonCommit)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagRequestToNonCommit(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -750,10 +737,9 @@ func testSuccessfulUserCreateTagRequestToNonCommit(t *testing.T, ctx context.Con
 }
 
 func TestSuccessfulUserCreateTagNestedTags(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testSuccessfulUserCreateTagNestedTags)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserCreateTagNestedTags(t *testing.T, ctx context.Context) {
 	locator := config.NewLocator(config.Config)
 
 	serverSocketPath, stop := runOperationServiceServer(t)
@@ -886,13 +872,10 @@ func testSuccessfulUserCreateTagNestedTags(t *testing.T, ctx context.Context) {
 	}
 }
 
-func TestUserCreateTag_stableTagIDs(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.GoUserCreateTag,
-	}).Run(t, testUserCreateTagStableTagIDs)
-}
+func TestUserCreateTagStableTagIDs(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUserCreateTagStableTagIDs(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -978,10 +961,9 @@ func testUserDeleteTagsuccessfulDeletionOfPrefixedTag(t *testing.T, ctx context.
 }
 
 func TestUserCreateTagsuccessfulCreationOfPrefixedTag(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testUserCreateTagsuccessfulCreationOfPrefixedTag)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testUserCreateTagsuccessfulCreationOfPrefixedTag(t *testing.T, ctx context.Context) {
 	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
 	defer cleanupFn()
 
@@ -1042,10 +1024,9 @@ func testUserCreateTagsuccessfulCreationOfPrefixedTag(t *testing.T, ctx context.
 }
 
 func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testSuccessfulGitHooksForUserCreateTagRequest)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -1207,10 +1188,9 @@ func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) {
 }
 
 func TestFailedUserCreateTagDueToHooks(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testFailedUserCreateTagDueToHooks)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserCreateTagDueToHooks(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -1240,10 +1220,9 @@ func testFailedUserCreateTagDueToHooks(t *testing.T, ctx context.Context) {
 }
 
 func TestFailedUserCreateTagRequestDueToTagExistence(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testFailedUserCreateTagRequestDueToTagExistence)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserCreateTagRequestDueToTagExistence(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -1299,10 +1278,9 @@ func testFailedUserCreateTagRequestDueToTagExistence(t *testing.T, ctx context.C
 }
 
 func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) {
-	testWithFeature(t, featureflag.GoUserCreateTag, testFailedUserCreateTagRequestDueToValidation)
-}
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runOperationServiceServer(t)
 	defer stop()
 
@@ -1427,7 +1405,6 @@ func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Con
 func TestTagHookOutput(t *testing.T) {
 	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
 		featureflag.ReferenceTransactions,
-		featureflag.GoUserCreateTag,
 	}).Run(t, testTagHookOutput)
 }
 
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index d4adc9ead..5f24c0aa2 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -17,8 +17,6 @@ var (
 	LogCommandStats = FeatureFlag{Name: "log_command_stats", OnByDefault: false}
 	// GoUserFFBranch enables the Go implementation of UserFFBranch
 	GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: false}
-	// GoUserCreateBranch enables the Go implementation of UserCreateBranch
-	GoUserCreateBranch = FeatureFlag{Name: "go_user_create_branch", OnByDefault: true}
 	// GoUserUpdateBranch enables the Go implementation of UserUpdateBranch
 	GoUserUpdateBranch = FeatureFlag{Name: "go_user_update_branch", OnByDefault: false}
 	// GoUserCommitFiles enables the Go implementation of UserCommitFiles
@@ -28,8 +26,6 @@ var (
 	// GoUserUpdateSubmodule enables the Go implementation of
 	// UserUpdateSubmodules
 	GoUserUpdateSubmodule = FeatureFlag{Name: "go_user_update_submodule", OnByDefault: false}
-	// GoUserCreateTag enables the Go implementation of UserCreateTag
-	GoUserCreateTag = FeatureFlag{Name: "go_user_create_tag", OnByDefault: true}
 	// GoUserRevert enables the Go implementation of UserRevert
 	GoUserRevert = FeatureFlag{Name: "go_user_revert", OnByDefault: false}
 
@@ -103,12 +99,10 @@ var All = []FeatureFlag{
 	LogCommandStats,
 	ReferenceTransactions,
 	GoUserFFBranch,
-	GoUserCreateBranch,
 	GoUserUpdateBranch,
 	GoUserCommitFiles,
 	GoResolveConflicts,
 	GoUserUpdateSubmodule,
-	GoUserCreateTag,
 	GoUserRevert,
 	TxApplyBfgObjectMapStream,
 	TxResolveConflicts,