Merge branch 'pks-go-fetch-source-branch-remove-ff' into 'master'

featureflag: Remove GoFetchSourceBranch feature flag

See merge request gitlab-org/gitaly!3050

5 files changed, 124 insertions, 185 deletions

diff --git a/changelogs/unreleased/pks-go-fetch-source-branch-remove-ff.yml b/changelogs/unreleased/pks-go-fetch-source-branch-remove-ff.yml
new file mode 100644
index 000000000..bf4d64441
--- /dev/null
+++ b/changelogs/unreleased/pks-go-fetch-source-branch-remove-ff.yml
@@ -0,0 +1,5 @@
+---
+title: 'featureflag: Remove GoFetchSourceBranch feature flag'
+merge_request: 3050
+author:
+type: performance
diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go
index bbf917dfc..d77a40ac2 100644
--- a/internal/gitaly/service/repository/fetch.go
+++ b/internal/gitaly/service/repository/fetch.go
@@ -7,18 +7,12 @@ import (
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/remoterepo"
-	"gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/gitalyssh"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourceBranchRequest) (*gitalypb.FetchSourceBranchResponse, error) {
-	if featureflag.IsDisabled(ctx, featureflag.GoFetchSourceBranch) {
-		return s.rubyFetchSourceBranch(ctx, req)
-	}
-
 	if err := git.ValidateRevision(req.GetSourceBranch()); err != nil {
 		return nil, helper.ErrInvalidArgument(err)
 	}
@@ -108,17 +102,3 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc
 
 	return &gitalypb.FetchSourceBranchResponse{Result: true}, nil
 }
-
-func (s *server) rubyFetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourceBranchRequest) (*gitalypb.FetchSourceBranchResponse, error) {
-	client, err := s.ruby.RepositoryServiceClient(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	clientCtx, err := rubyserver.SetHeaders(ctx, s.locator, req.GetRepository())
-	if err != nil {
-		return nil, err
-	}
-
-	return client.FetchSourceBranch(clientCtx, req)
-}
diff --git a/internal/gitaly/service/repository/fetch_test.go b/internal/gitaly/service/repository/fetch_test.go
index 725150c9e..f2e6a5f9e 100644
--- a/internal/gitaly/service/repository/fetch_test.go
+++ b/internal/gitaly/service/repository/fetch_test.go
@@ -14,7 +14,6 @@ import (
 	serverPkg "gitlab.com/gitlab-org/gitaly/internal/gitaly/server"
 	hookservice "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/service/repository"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -32,55 +31,36 @@ func TestFetchSourceBranchSourceRepositorySuccess(t *testing.T) {
 	client, conn := repository.NewRepositoryClient(t, serverSocketPath)
 	defer conn.Close()
 
-	for _, tc := range []struct {
-		desc             string
-		disabledFeatures []featureflag.FeatureFlag
-	}{
-		{
-			desc: "go",
-		},
-		{
-			desc:             "ruby",
-			disabledFeatures: []featureflag.FeatureFlag{featureflag.GoFetchSourceBranch},
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
-			md := testhelper.GitalyServersMetadata(t, serverSocketPath)
-			ctx = testhelper.MergeOutgoingMetadata(ctx, md)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-			for _, feature := range tc.disabledFeatures {
-				ctx = featureflag.OutgoingCtxWithFeatureFlagValue(ctx, feature, "true")
-			}
+	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+	ctx = testhelper.MergeOutgoingMetadata(ctx, md)
 
-			targetRepo, _, cleanup := newTestRepo(t, locator, "fetch-source-target.git")
-			defer cleanup()
+	targetRepo, _, cleanup := newTestRepo(t, locator, "fetch-source-target.git")
+	defer cleanup()
 
-			sourceRepo, sourcePath, cleanup := newTestRepo(t, locator, "fetch-source-source.git")
-			defer cleanup()
+	sourceRepo, sourcePath, cleanup := newTestRepo(t, locator, "fetch-source-source.git")
+	defer cleanup()
 
-			sourceBranch := "fetch-source-branch-test-branch"
-			newCommitID := testhelper.CreateCommit(t, sourcePath, sourceBranch, nil)
+	sourceBranch := "fetch-source-branch-test-branch"
+	newCommitID := testhelper.CreateCommit(t, sourcePath, sourceBranch, nil)
 
-			targetRef := "refs/tmp/fetch-source-branch-test"
-			req := &gitalypb.FetchSourceBranchRequest{
-				Repository:       targetRepo,
-				SourceRepository: sourceRepo,
-				SourceBranch:     []byte(sourceBranch),
-				TargetRef:        []byte(targetRef),
-			}
+	targetRef := "refs/tmp/fetch-source-branch-test"
+	req := &gitalypb.FetchSourceBranchRequest{
+		Repository:       targetRepo,
+		SourceRepository: sourceRepo,
+		SourceBranch:     []byte(sourceBranch),
+		TargetRef:        []byte(targetRef),
+	}
 
-			resp, err := client.FetchSourceBranch(ctx, req)
-			require.NoError(t, err)
-			require.True(t, resp.Result, "response.Result should be true")
+	resp, err := client.FetchSourceBranch(ctx, req)
+	require.NoError(t, err)
+	require.True(t, resp.Result, "response.Result should be true")
 
-			fetchedCommit, err := gitLog.GetCommit(ctx, locator, targetRepo, git.Revision(targetRef))
-			require.NoError(t, err)
-			require.Equal(t, newCommitID, fetchedCommit.GetId())
-		})
-	}
+	fetchedCommit, err := gitLog.GetCommit(ctx, locator, targetRepo, git.Revision(targetRef))
+	require.NoError(t, err)
+	require.Equal(t, newCommitID, fetchedCommit.GetId())
 }
 
 func TestFetchSourceBranchSameRepositorySuccess(t *testing.T) {
@@ -92,52 +72,33 @@ func TestFetchSourceBranchSameRepositorySuccess(t *testing.T) {
 	client, conn := repository.NewRepositoryClient(t, serverSocketPath)
 	defer conn.Close()
 
-	for _, tc := range []struct {
-		desc             string
-		disabledFeatures []featureflag.FeatureFlag
-	}{
-		{
-			desc: "go",
-		},
-		{
-			desc:             "ruby",
-			disabledFeatures: []featureflag.FeatureFlag{featureflag.GoFetchSourceBranch},
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
-			md := testhelper.GitalyServersMetadata(t, serverSocketPath)
-			ctx = testhelper.MergeOutgoingMetadata(ctx, md)
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-			for _, feature := range tc.disabledFeatures {
-				ctx = featureflag.OutgoingCtxWithFeatureFlagValue(ctx, feature, "false")
-			}
+	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+	ctx = testhelper.MergeOutgoingMetadata(ctx, md)
 
-			repo, repoPath, cleanup := newTestRepo(t, locator, "fetch-source-source.git")
-			defer cleanup()
+	repo, repoPath, cleanup := newTestRepo(t, locator, "fetch-source-source.git")
+	defer cleanup()
 
-			sourceBranch := "fetch-source-branch-test-branch"
-			newCommitID := testhelper.CreateCommit(t, repoPath, sourceBranch, nil)
+	sourceBranch := "fetch-source-branch-test-branch"
+	newCommitID := testhelper.CreateCommit(t, repoPath, sourceBranch, nil)
 
-			targetRef := "refs/tmp/fetch-source-branch-test"
-			req := &gitalypb.FetchSourceBranchRequest{
-				Repository:       repo,
-				SourceRepository: repo,
-				SourceBranch:     []byte(sourceBranch),
-				TargetRef:        []byte(targetRef),
-			}
+	targetRef := "refs/tmp/fetch-source-branch-test"
+	req := &gitalypb.FetchSourceBranchRequest{
+		Repository:       repo,
+		SourceRepository: repo,
+		SourceBranch:     []byte(sourceBranch),
+		TargetRef:        []byte(targetRef),
+	}
 
-			resp, err := client.FetchSourceBranch(ctx, req)
-			require.NoError(t, err)
-			require.True(t, resp.Result, "response.Result should be true")
+	resp, err := client.FetchSourceBranch(ctx, req)
+	require.NoError(t, err)
+	require.True(t, resp.Result, "response.Result should be true")
 
-			fetchedCommit, err := gitLog.GetCommit(ctx, locator, repo, git.Revision(targetRef))
-			require.NoError(t, err)
-			require.Equal(t, newCommitID, fetchedCommit.GetId())
-		})
-	}
+	fetchedCommit, err := gitLog.GetCommit(ctx, locator, repo, git.Revision(targetRef))
+	require.NoError(t, err)
+	require.Equal(t, newCommitID, fetchedCommit.GetId())
 }
 
 func TestFetchSourceBranchBranchNotFound(t *testing.T) {
@@ -149,69 +110,50 @@ func TestFetchSourceBranchBranchNotFound(t *testing.T) {
 	client, conn := repository.NewRepositoryClient(t, serverSocketPath)
 	defer conn.Close()
 
-	for _, tc := range []struct {
-		desc             string
-		disabledFeatures []featureflag.FeatureFlag
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+	ctx = testhelper.MergeOutgoingMetadata(ctx, md)
+
+	targetRepo, _, cleanup := newTestRepo(t, locator, "fetch-source-target.git")
+	defer cleanup()
+
+	sourceRepo, _, cleanup := newTestRepo(t, locator, "fetch-source-source.git")
+	defer cleanup()
+
+	sourceBranch := "does-not-exist"
+	targetRef := "refs/tmp/fetch-source-branch-test"
+
+	testCases := []struct {
+		req  *gitalypb.FetchSourceBranchRequest
+		desc string
 	}{
 		{
-			desc: "go",
+			desc: "target different from source",
+			req: &gitalypb.FetchSourceBranchRequest{
+				Repository:       targetRepo,
+				SourceRepository: sourceRepo,
+				SourceBranch:     []byte(sourceBranch),
+				TargetRef:        []byte(targetRef),
+			},
 		},
 		{
-			desc:             "ruby",
-			disabledFeatures: []featureflag.FeatureFlag{featureflag.GoFetchSourceBranch},
+			desc: "target same as source",
+			req: &gitalypb.FetchSourceBranchRequest{
+				Repository:       sourceRepo,
+				SourceRepository: sourceRepo,
+				SourceBranch:     []byte(sourceBranch),
+				TargetRef:        []byte(targetRef),
+			},
 		},
-	} {
+	}
+
+	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
-			md := testhelper.GitalyServersMetadata(t, serverSocketPath)
-			ctx = testhelper.MergeOutgoingMetadata(ctx, md)
-
-			for _, feature := range tc.disabledFeatures {
-				ctx = featureflag.OutgoingCtxWithFeatureFlagValue(ctx, feature, "false")
-			}
-
-			targetRepo, _, cleanup := newTestRepo(t, locator, "fetch-source-target.git")
-			defer cleanup()
-
-			sourceRepo, _, cleanup := newTestRepo(t, locator, "fetch-source-source.git")
-			defer cleanup()
-
-			sourceBranch := "does-not-exist"
-			targetRef := "refs/tmp/fetch-source-branch-test"
-
-			testCases := []struct {
-				req  *gitalypb.FetchSourceBranchRequest
-				desc string
-			}{
-				{
-					desc: "target different from source",
-					req: &gitalypb.FetchSourceBranchRequest{
-						Repository:       targetRepo,
-						SourceRepository: sourceRepo,
-						SourceBranch:     []byte(sourceBranch),
-						TargetRef:        []byte(targetRef),
-					},
-				},
-				{
-					desc: "target same as source",
-					req: &gitalypb.FetchSourceBranchRequest{
-						Repository:       sourceRepo,
-						SourceRepository: sourceRepo,
-						SourceBranch:     []byte(sourceBranch),
-						TargetRef:        []byte(targetRef),
-					},
-				},
-			}
-
-			for _, tc := range testCases {
-				t.Run(tc.desc, func(t *testing.T) {
-					resp, err := client.FetchSourceBranch(ctx, tc.req)
-					require.NoError(t, err)
-					require.False(t, resp.Result, "response.Result should be false")
-				})
-			}
+			resp, err := client.FetchSourceBranch(ctx, tc.req)
+			require.NoError(t, err)
+			require.False(t, resp.Result, "response.Result should be false")
 		})
 	}
 }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 3ac1cfa7d..d4adc9ead 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 (
-	// GoFetchSourceBranch enables a go implementation of FetchSourceBranch
-	GoFetchSourceBranch = FeatureFlag{Name: "go_fetch_source_branch", OnByDefault: true}
 	// DistributedReads allows praefect to redirect accessor operations to up-to-date secondaries
 	DistributedReads = FeatureFlag{Name: "distributed_reads", OnByDefault: true}
 	// ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency
@@ -101,7 +99,6 @@ var (
 
 // All includes all feature flags.
 var All = []FeatureFlag{
-	GoFetchSourceBranch,
 	DistributedReads,
 	LogCommandStats,
 	ReferenceTransactions,
diff --git a/internal/testhelper/featureset_test.go b/internal/testhelper/featureset_test.go
index fc6fa0f60..5a118c870 100644
--- a/internal/testhelper/featureset_test.go
+++ b/internal/testhelper/featureset_test.go
@@ -9,6 +9,11 @@ import (
 	ff "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 )
 
+var (
+	featureFlagA = ff.FeatureFlag{Name: "test_feature_flag_a"}
+	featureFlagB = ff.FeatureFlag{Name: "test_feature_flag_b"}
+)
+
 func features(flag ...ff.FeatureFlag) map[ff.FeatureFlag]struct{} {
 	features := make(map[ff.FeatureFlag]struct{}, len(flag))
 	for _, f := range flag {
@@ -26,43 +31,43 @@ func TestNewFeatureSets(t *testing.T) {
 	}{
 		{
 			desc:     "single Go feature flag",
-			features: []ff.FeatureFlag{ff.GoFetchSourceBranch},
+			features: []ff.FeatureFlag{featureFlagA},
 			expected: FeatureSets{
 				FeatureSet{
 					features:     features(),
 					rubyFeatures: features(),
 				},
 				FeatureSet{
-					features:     features(ff.GoFetchSourceBranch),
+					features:     features(featureFlagA),
 					rubyFeatures: features(),
 				},
 			},
 		},
 		{
 			desc:     "two Go feature flags",
-			features: []ff.FeatureFlag{ff.GoFetchSourceBranch, ff.DistributedReads},
+			features: []ff.FeatureFlag{featureFlagA, featureFlagB},
 			expected: FeatureSets{
 				FeatureSet{
 					features:     features(),
 					rubyFeatures: features(),
 				},
 				FeatureSet{
-					features:     features(ff.DistributedReads),
+					features:     features(featureFlagB),
 					rubyFeatures: features(),
 				},
 				FeatureSet{
-					features:     features(ff.GoFetchSourceBranch),
+					features:     features(featureFlagA),
 					rubyFeatures: features(),
 				},
 				FeatureSet{
-					features:     features(ff.DistributedReads, ff.GoFetchSourceBranch),
+					features:     features(featureFlagB, featureFlagA),
 					rubyFeatures: features(),
 				},
 			},
 		},
 		{
 			desc:         "single Ruby feature flag",
-			rubyFeatures: []ff.FeatureFlag{ff.GoFetchSourceBranch},
+			rubyFeatures: []ff.FeatureFlag{featureFlagA},
 			expected: FeatureSets{
 				FeatureSet{
 					features:     features(),
@@ -70,13 +75,13 @@ func TestNewFeatureSets(t *testing.T) {
 				},
 				FeatureSet{
 					features:     features(),
-					rubyFeatures: features(ff.GoFetchSourceBranch),
+					rubyFeatures: features(featureFlagA),
 				},
 			},
 		},
 		{
 			desc:         "two Ruby feature flags",
-			rubyFeatures: []ff.FeatureFlag{ff.GoFetchSourceBranch, ff.DistributedReads},
+			rubyFeatures: []ff.FeatureFlag{featureFlagA, featureFlagB},
 			expected: FeatureSets{
 				FeatureSet{
 					features:     features(),
@@ -84,38 +89,38 @@ func TestNewFeatureSets(t *testing.T) {
 				},
 				FeatureSet{
 					features:     features(),
-					rubyFeatures: features(ff.DistributedReads),
+					rubyFeatures: features(featureFlagB),
 				},
 				FeatureSet{
 					features:     features(),
-					rubyFeatures: features(ff.GoFetchSourceBranch),
+					rubyFeatures: features(featureFlagA),
 				},
 				FeatureSet{
 					features:     features(),
-					rubyFeatures: features(ff.DistributedReads, ff.GoFetchSourceBranch),
+					rubyFeatures: features(featureFlagB, featureFlagA),
 				},
 			},
 		},
 		{
 			desc:         "Go and Ruby feature flag",
-			features:     []ff.FeatureFlag{ff.DistributedReads},
-			rubyFeatures: []ff.FeatureFlag{ff.GoFetchSourceBranch},
+			features:     []ff.FeatureFlag{featureFlagB},
+			rubyFeatures: []ff.FeatureFlag{featureFlagA},
 			expected: FeatureSets{
 				FeatureSet{
 					features:     features(),
 					rubyFeatures: features(),
 				},
 				FeatureSet{
-					features:     features(ff.DistributedReads),
+					features:     features(featureFlagB),
 					rubyFeatures: features(),
 				},
 				FeatureSet{
 					features:     features(),
-					rubyFeatures: features(ff.GoFetchSourceBranch),
+					rubyFeatures: features(featureFlagA),
 				},
 				FeatureSet{
-					features:     features(ff.DistributedReads),
-					rubyFeatures: features(ff.GoFetchSourceBranch),
+					features:     features(featureFlagB),
+					rubyFeatures: features(featureFlagA),
 				},
 			},
 		},
@@ -135,17 +140,27 @@ func TestNewFeatureSets(t *testing.T) {
 func TestFeatureSets_Run(t *testing.T) {
 	var flags [][2]bool
 
+	// This test depends on feature flags being default-enabled in the test
+	// context, which requires those flags to exist in the ff.All slice. So
+	// let's just append them here so we do not need to use a "real"
+	// feature flag, as that would require constant change when we remove
+	// old feature flags.
+	defer func(old []ff.FeatureFlag) {
+		ff.All = old
+	}(ff.All)
+	ff.All = append(ff.All, featureFlagA, featureFlagB)
+
 	NewFeatureSets([]ff.FeatureFlag{
-		ff.DistributedReads, ff.GoFetchSourceBranch,
+		featureFlagB, featureFlagA,
 	}).Run(t, func(t *testing.T, ctx context.Context) {
 		ctx = helper.OutgoingToIncoming(ctx)
 		flags = append(flags, [2]bool{
-			ff.IsDisabled(ctx, ff.DistributedReads),
-			ff.IsDisabled(ctx, ff.GoFetchSourceBranch),
+			ff.IsDisabled(ctx, featureFlagB),
+			ff.IsDisabled(ctx, featureFlagA),
 		})
 	})
 
-	require.Equal(t, flags, [][2]bool{
+	require.ElementsMatch(t, flags, [][2]bool{
 		{false, false},
 		{true, false},
 		{false, true},