Merge branch 'zj-remove-update-submodule-ff' into 'master'

operations: Remove update submodule feature flag

Closes #3380

See merge request gitlab-org/gitaly!3451

4 files changed, 29 insertions, 96 deletions

diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go
index fc5f79bf1..828569c9b 100644
--- a/internal/gitaly/service/operations/submodules.go
+++ b/internal/gitaly/service/operations/submodules.go
@@ -12,9 +12,7 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"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"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -27,21 +25,7 @@ func (s *Server) UserUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		return nil, status.Errorf(codes.InvalidArgument, userUpdateSubmoduleName+": %v", err)
 	}
 
-	if featureflag.IsEnabled(ctx, featureflag.GoUserUpdateSubmodule) {
-		return s.userUpdateSubmodule(ctx, req)
-	}
-
-	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.UserUpdateSubmodule(clientCtx, req)
+	return s.userUpdateSubmodule(ctx, req)
 }
 
 func validateUserUpdateSubmoduleRequest(req *gitalypb.UserUpdateSubmoduleRequest) error {
diff --git a/internal/gitaly/service/operations/submodules_test.go b/internal/gitaly/service/operations/submodules_test.go
index 454c40f46..c2d68c0f0 100644
--- a/internal/gitaly/service/operations/submodules_test.go
+++ b/internal/gitaly/service/operations/submodules_test.go
@@ -2,7 +2,6 @@ package operations
 
 import (
 	"bytes"
-	"context"
 	"fmt"
 	"testing"
 
@@ -12,24 +11,16 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/git/lstree"
-	"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 testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testSuccessfulUserUpdateSubmoduleRequestFeatured(t, ctx, cfg, rubySrv)
-	})
-}
+func TestSuccessfulUserUpdateSubmoduleRequest(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testSuccessfulUserUpdateSubmoduleRequestFeatured(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)
 
@@ -106,17 +97,11 @@ func testSuccessfulUserUpdateSubmoduleRequestFeatured(t *testing.T, ctx context.
 	}
 }
 
-func testUserUpdateSubmoduleStableID(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testUserUpdateSubmoduleStableIDFeatured(t, ctx, cfg, rubySrv)
-	})
-}
-
-func testUserUpdateSubmoduleStableIDFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, cfg, repoProto, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+func TestUserUpdateSubmoduleStableID(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
+	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	response, err := client.UserUpdateSubmodule(ctx, &gitalypb.UserUpdateSubmoduleRequest{
@@ -158,16 +143,11 @@ func testUserUpdateSubmoduleStableIDFeatured(t *testing.T, ctx context.Context,
 	}, commit)
 }
 
-func testFailedUserUpdateSubmoduleRequestDueToValidations(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testFailedUserUpdateSubmoduleRequestDueToValidationsFeatured(t, ctx, cfg, rubySrv)
-	})
-}
+func TestFailedUserUpdateSubmoduleRequestDueToValidations(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserUpdateSubmoduleRequestDueToValidationsFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, _, repo, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
 		desc    string
@@ -281,16 +261,11 @@ func testFailedUserUpdateSubmoduleRequestDueToValidationsFeatured(t *testing.T,
 	}
 }
 
-func testFailedUserUpdateSubmoduleRequestDueToInvalidBranch(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testFailedUserUpdateSubmoduleRequestDueToInvalidBranchFeatured(t, ctx, cfg, rubySrv)
-	})
-}
+func TestFailedUserUpdateSubmoduleRequestDueToInvalidBranch(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserUpdateSubmoduleRequestDueToInvalidBranchFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, _, repo, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	request := &gitalypb.UserUpdateSubmoduleRequest{
 		Repository:    repo,
@@ -306,16 +281,11 @@ func testFailedUserUpdateSubmoduleRequestDueToInvalidBranchFeatured(t *testing.T
 	require.Contains(t, err.Error(), "Cannot find branch")
 }
 
-func testFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testFailedUserUpdateSubmoduleRequestDueToInvalidSubmoduleFeatured(t, ctx, cfg, rubySrv)
-	})
-}
+func TestFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserUpdateSubmoduleRequestDueToInvalidSubmoduleFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, _, repo, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	request := &gitalypb.UserUpdateSubmoduleRequest{
 		Repository:    repo,
@@ -331,16 +301,11 @@ func testFailedUserUpdateSubmoduleRequestDueToInvalidSubmoduleFeatured(t *testin
 	require.Equal(t, response.CommitError, "Invalid submodule path")
 }
 
-func testFailedUserUpdateSubmoduleRequestDueToSameReference(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testFailedUserUpdateSubmoduleRequestDueToSameReferenceFeatured(t, ctx, cfg, rubySrv)
-	})
-}
+func TestFailedUserUpdateSubmoduleRequestDueToSameReference(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserUpdateSubmoduleRequestDueToSameReferenceFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, _, repo, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	request := &gitalypb.UserUpdateSubmoduleRequest{
 		Repository:    repo,
@@ -359,16 +324,11 @@ func testFailedUserUpdateSubmoduleRequestDueToSameReferenceFeatured(t *testing.T
 	require.Contains(t, response.CommitError, "is already at")
 }
 
-func testFailedUserUpdateSubmoduleRequestDueToRepositoryEmpty(t *testing.T, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	testhelper.NewFeatureSets(
-		[]featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule},
-	).Run(t, func(t *testing.T, ctx context.Context) {
-		testFailedUserUpdateSubmoduleRequestDueToRepositoryEmptyFeatured(t, ctx, cfg, rubySrv)
-	})
-}
+func TestFailedUserUpdateSubmoduleRequestDueToRepositoryEmpty(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testFailedUserUpdateSubmoduleRequestDueToRepositoryEmptyFeatured(t *testing.T, ctx context.Context, cfg config.Cfg, rubySrv *rubyserver.Server) {
-	ctx, _, _, _, client := setupOperationsServiceWithRuby(t, ctx, cfg, rubySrv)
+	ctx, cfg, _, _, client := setupOperationsService(t, ctx)
 
 	repo, _, cleanup := gittest.InitRepoWithWorktreeAtStorage(t, cfg, cfg.Storages[0])
 	t.Cleanup(cleanup)
diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go
index c2c3560ad..973d09ead 100644
--- a/internal/gitaly/service/operations/testhelper_test.go
+++ b/internal/gitaly/service/operations/testhelper_test.go
@@ -85,13 +85,6 @@ func TestWithRubySidecar(t *testing.T) {
 		testRebaseRequestWithDeletedFile,
 		testRebaseOntoRemoteBranch,
 		testRebaseFailedWithCode,
-		testSuccessfulUserUpdateSubmoduleRequest,
-		testUserUpdateSubmoduleStableID,
-		testFailedUserUpdateSubmoduleRequestDueToValidations,
-		testFailedUserUpdateSubmoduleRequestDueToInvalidBranch,
-		testFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule,
-		testFailedUserUpdateSubmoduleRequestDueToSameReference,
-		testFailedUserUpdateSubmoduleRequestDueToRepositoryEmpty,
 		testServerUserRevertFailedDueToCreateTreeErrorEmpty,
 	}
 	for _, f := range fs {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index a79c89f84..61da0e274 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -17,9 +17,6 @@ var (
 	GoUserUpdateBranch = FeatureFlag{Name: "go_user_update_branch", OnByDefault: true}
 	// UserRebaseConfirmable
 	GoUserRebaseConfirmable = FeatureFlag{Name: "go_user_rebase_confirmable", OnByDefault: false}
-	// GoUserUpdateSubmodule enables the Go implementation of
-	// UserUpdateSubmodules
-	GoUserUpdateSubmodule = FeatureFlag{Name: "go_user_update_submodule", OnByDefault: true}
 	// GoUserRevert enables the Go implementation of UserRevert
 	GoUserRevert = FeatureFlag{Name: "go_user_revert", OnByDefault: true}
 	// GoUpdateRemoteMirror enables the Go implementation of UpdateRemoteMirror
@@ -36,7 +33,6 @@ var All = []FeatureFlag{
 	ReferenceTransactions,
 	GoUserUpdateBranch,
 	GoUserRebaseConfirmable,
-	GoUserUpdateSubmodule,
 	GoUserRevert,
 	GrpcTreeEntryNotFound,
 	GoUpdateRemoteMirror,