Merge branch 'remove_ff_gitaly_go_user_revert' into 'master'

Remove gitaly feature flag gitaly_go_user_revert

See merge request gitlab-org/gitaly!3516

5 files changed, 41 insertions, 80 deletions

diff --git a/changelogs/unreleased/remove_ff_gitaly_go_user_revert.yml b/changelogs/unreleased/remove_ff_gitaly_go_user_revert.yml
new file mode 100644
index 000000000..4c8633b75
--- /dev/null
+++ b/changelogs/unreleased/remove_ff_gitaly_go_user_revert.yml
@@ -0,0 +1,5 @@
+---
+title: Remove gitaly feature flag gitaly_go_user_revert
+merge_request: 3516
+author:
+type: changed
diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go
index 54f97990e..aba4beaeb 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -10,9 +10,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/remoterepo"
 	"gitlab.com/gitlab-org/gitaly/internal/git2go"
-	"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"
 )
 
@@ -20,9 +18,6 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	if err := validateCherryPickOrRevertRequest(req); err != nil {
 		return nil, helper.ErrInvalidArgument(err)
 	}
-	if featureflag.IsDisabled(ctx, featureflag.GoUserRevert) {
-		return s.rubyUserRevert(ctx, req)
-	}
 
 	startRevision, err := s.fetchStartRevision(ctx, req)
 	if err != nil {
@@ -128,20 +123,6 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 	}, nil
 }
 
-func (s *Server) rubyUserRevert(ctx context.Context, req *gitalypb.UserRevertRequest) (*gitalypb.UserRevertResponse, 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.UserRevert(clientCtx, req)
-}
-
 type requestFetchingStartRevision interface {
 	GetRepository() *gitalypb.Repository
 	GetBranchName() []byte
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index c2db91fa3..6d45072eb 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"testing"
 
 	"github.com/golang/protobuf/ptypes/timestamp"
@@ -9,20 +8,16 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 )
 
-func testServerUserRevertSuccessful(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertSuccessfulFeatured)
-}
+func TestServer_UserRevert_successful(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertSuccessfulFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -174,12 +169,11 @@ func testServerUserRevertSuccessfulFeatured(t *testing.T, ctx context.Context, c
 	}
 }
 
-func testServerUserRevertStableID(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertStableIDFeatured)
-}
+func TestServer_UserRevert_stableID(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertStableIDFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -229,12 +223,11 @@ func testServerUserRevertStableIDFeatured(t *testing.T, ctx context.Context, cfg
 	}, revertedCommit)
 }
 
-func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertSuccessfulIntoNewRepo)
-}
+func TestServer_UserRevert_successfulIntoEmptyRepo(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertSuccessfulIntoNewRepo(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, startRepoProto, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, startRepoProto, _, client := setupOperationsService(t, ctx)
 
 	startRepo := localrepo.NewTestRepo(t, cfg, startRepoProto)
 
@@ -277,12 +270,11 @@ func testServerUserRevertSuccessfulIntoNewRepo(t *testing.T, ctx context.Context
 	require.Equal(t, masterHeadCommit.Id, headCommit.ParentIds[0])
 }
 
-func testServerUserRevertSuccessfulGitHooks(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertSuccessfulGitHooksFeatured)
-}
+func TestServer_UserRevert_successfulGitHooks(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertSuccessfulGitHooksFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -316,12 +308,11 @@ func testServerUserRevertSuccessfulGitHooksFeatured(t *testing.T, ctx context.Co
 	}
 }
 
-func testServerUserRevertFailuedDueToValidations(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertFailuedDueToValidationsFeatured)
-}
+func TestServer_UserRevert_failuedDueToValidations(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertFailuedDueToValidationsFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -389,12 +380,11 @@ func testServerUserRevertFailuedDueToValidationsFeatured(t *testing.T, ctx conte
 	}
 }
 
-func testServerUserRevertFailedDueToPreReceiveError(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertFailedDueToPreReceiveErrorFeatured)
-}
+func TestServer_UserRevert_failedDueToPreReceiveError(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertFailedDueToPreReceiveErrorFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -425,12 +415,11 @@ func testServerUserRevertFailedDueToPreReceiveErrorFeatured(t *testing.T, ctx co
 	}
 }
 
-func testServerUserRevertFailedDueToCreateTreeErrorConflict(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertFailedDueToCreateTreeErrorConflictFeatured)
-}
+func TestServer_UserRevert_failedDueToCreateTreeErrorConflict(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertFailedDueToCreateTreeErrorConflictFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -455,12 +444,11 @@ func testServerUserRevertFailedDueToCreateTreeErrorConflictFeatured(t *testing.T
 	require.Equal(t, gitalypb.UserRevertResponse_CONFLICT, response.CreateTreeErrorCode)
 }
 
-func testServerUserRevertFailedDueToCreateTreeErrorEmpty(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertFailedDueToCreateTreeErrorEmptyFeatured)
-}
+func TestServer_UserRevert_failedDueToCreateTreeErrorEmpty(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertFailedDueToCreateTreeErrorEmptyFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -489,12 +477,11 @@ func testServerUserRevertFailedDueToCreateTreeErrorEmptyFeatured(t *testing.T, c
 	require.Equal(t, gitalypb.UserRevertResponse_EMPTY, response.CreateTreeErrorCode)
 }
 
-func testServerUserRevertFailedDueToCommitError(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testWithFeature(t, featureflag.GoUserRevert, cfg, rubySrv, testServerUserRevertFailedDueToCommitErrorFeatured)
-}
+func TestServer_UserRevert_failedDueToCommitError(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testServerUserRevertFailedDueToCommitErrorFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, repoPath, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go
index f20d05c96..db8338c06 100644
--- a/internal/gitaly/service/operations/testhelper_test.go
+++ b/internal/gitaly/service/operations/testhelper_test.go
@@ -61,14 +61,6 @@ func TestWithRubySidecar(t *testing.T) {
 		testSuccessfulUserApplyPatch,
 		testUserApplyPatchStableID,
 		testFailedPatchApplyPatch,
-		testServerUserRevertSuccessful,
-		testServerUserRevertStableID,
-		testServerUserRevertSuccessfulIntoEmptyRepo,
-		testServerUserRevertSuccessfulGitHooks,
-		testServerUserRevertFailuedDueToValidations,
-		testServerUserRevertFailedDueToPreReceiveError,
-		testServerUserRevertFailedDueToCreateTreeErrorConflict,
-		testServerUserRevertFailedDueToCommitError,
 		testSuccessfulUserUpdateBranchRequestToDelete,
 		testSuccessfulGitHooksForUserUpdateBranchRequest,
 		testFailedUserUpdateBranchDueToHooks,
@@ -85,7 +77,6 @@ func TestWithRubySidecar(t *testing.T) {
 		testRebaseRequestWithDeletedFile,
 		testRebaseOntoRemoteBranch,
 		testRebaseFailedWithCode,
-		testServerUserRevertFailedDueToCreateTreeErrorEmpty,
 	}
 	for _, f := range fs {
 		t.Run(runtime.FuncForPC(reflect.ValueOf(f).Pointer()).Name(), func(t *testing.T) {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 4fd6495b8..7c6d615cb 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -17,8 +17,6 @@ var (
 	GoUserUpdateBranch = FeatureFlag{Name: "go_user_update_branch", OnByDefault: true}
 	// UserRebaseConfirmable
 	GoUserRebaseConfirmable = FeatureFlag{Name: "go_user_rebase_confirmable", OnByDefault: true}
-	// GoUserRevert enables the Go implementation of UserRevert
-	GoUserRevert = FeatureFlag{Name: "go_user_revert", OnByDefault: true}
 	// GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror
 	GoUpdateRemoteMirror = FeatureFlag{Name: "go_update_remote_mirror", OnByDefault: false}
 	// GrpcTreeEntryNotFound makes the TreeEntry gRPC call return NotFound instead of an empty blob
@@ -35,7 +33,6 @@ var All = []FeatureFlag{
 	ReferenceTransactions,
 	GoUserUpdateBranch,
 	GoUserRebaseConfirmable,
-	GoUserRevert,
 	GrpcTreeEntryNotFound,
 	GoUpdateRemoteMirror,
 	FetchInternalRemoteErrors,