Merge branch 'rs-ls-remote-by-default' into 'master'

Enable ruby_remote_branches_ls_remote flag by default

See merge request gitlab-org/gitaly!2330

7 files changed, 35 insertions, 16 deletions

diff --git a/changelogs/unreleased/rs-ls-remote-by-default.yml b/changelogs/unreleased/rs-ls-remote-by-default.yml
new file mode 100644
index 000000000..22d79e1a4
--- /dev/null
+++ b/changelogs/unreleased/rs-ls-remote-by-default.yml
@@ -0,0 +1,5 @@
+---
+title: Gather remote branches via ls-remote, rather than fetch, by default
+merge_request: 2330
+author:
+type: changed
diff --git a/internal/metadata/featureflag/context.go b/internal/metadata/featureflag/context.go
index cca196ef6..6522b4018 100644
--- a/internal/metadata/featureflag/context.go
+++ b/internal/metadata/featureflag/context.go
@@ -24,6 +24,23 @@ func OutgoingCtxWithFeatureFlags(ctx context.Context, flags ...FeatureFlag) cont
 	return metadata.NewOutgoingContext(ctx, md)
 }
 
+// OutgoingCtxWithDisabledFeatureFlags is used to explicitly disable "on by
+// default" feature flags in the outgoing context metadata. The returned context
+// is meant to be used in a client where the outcoming context is transferred to
+// an incoming context.
+func OutgoingCtxWithDisabledFeatureFlags(ctx context.Context, flags ...FeatureFlag) context.Context {
+	md, ok := metadata.FromOutgoingContext(ctx)
+	if !ok {
+		md = metadata.New(map[string]string{})
+	}
+
+	for _, flag := range flags {
+		md.Set(HeaderKey(flag.Name), "false")
+	}
+
+	return metadata.NewOutgoingContext(ctx, md)
+}
+
 // OutgoingCtxWithFeatureFlagValue is used to set feature flags with an explicit value.
 // only "true" or "false" are valid values. Any other value will be ignored.
 func OutgoingCtxWithFeatureFlagValue(ctx context.Context, flag FeatureFlag, val string) context.Context {
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 4e3b04c4f..0c0c2a498 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -14,7 +14,7 @@ var (
 	// GoUpdateHook will bypass the ruby update hook and use the go implementation of custom hooks
 	GoUpdateHook = FeatureFlag{Name: "go_update_hook", OnByDefault: false}
 	// RemoteBranchesLsRemote will use `ls-remote` for remote branches
-	RemoteBranchesLsRemote = FeatureFlag{Name: "ruby_remote_branches_ls_remote", OnByDefault: false}
+	RemoteBranchesLsRemote = FeatureFlag{Name: "ruby_remote_branches_ls_remote", OnByDefault: true}
 	// GoFetchSourceBranch enables a go implementation of FetchSourceBranch
 	GoFetchSourceBranch = FeatureFlag{Name: "go_fetch_source_branch", OnByDefault: false}
 	// ReferenceTransactions will handle Git reference updates via the transaction service for strong consistency
diff --git a/internal/service/remote/update_remote_mirror_test.go b/internal/service/remote/update_remote_mirror_test.go
index 1c87b5e2b..30a672997 100644
--- a/internal/service/remote/update_remote_mirror_test.go
+++ b/internal/service/remote/update_remote_mirror_test.go
@@ -28,11 +28,11 @@ func TestSuccessfulUpdateRemoteMirrorRequest(t *testing.T) {
 	}{
 		{
 			"ls-remote",
-			featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote),
+			ctx,
 		},
 		{
 			"fetch-remote",
-			ctx,
+			featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote),
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -174,9 +174,6 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithLsRemote(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	// Enable the flag to use ls-remote
-	ctx = featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote)
-
 	firstRequest := &gitalypb.UpdateRemoteMirrorRequest{
 		Repository:           testRepo,
 		RefName:              remoteName,
@@ -232,11 +229,11 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithWildcards(t *testing.T) {
 	}{
 		{
 			"ls-remote",
-			featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote),
+			ctx,
 		},
 		{
 			"fetch-remote",
-			ctx,
+			featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote),
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -328,11 +325,11 @@ func TestSuccessfulUpdateRemoteMirrorRequestWithKeepDivergentRefs(t *testing.T)
 	}{
 		{
 			"ls-remote",
-			featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote),
+			ctx,
 		},
 		{
 			"fetch-remote",
-			ctx,
+			featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote),
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
@@ -445,8 +442,8 @@ func TestFailedUpdateRemoteMirrorRequestDueToValidation(t *testing.T) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
-			// Ensure this flag doesn't alter existing behavior
-			ctx = featureflag.OutgoingCtxWithFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote)
+			// Ensure disabling this flag doesn't alter previous behavior
+			ctx = featureflag.OutgoingCtxWithDisabledFeatureFlags(ctx, featureflag.RemoteBranchesLsRemote)
 
 			stream, err := client.UpdateRemoteMirror(ctx)
 			require.NoError(t, err)
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index 3c6491817..28502287c 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -84,8 +84,8 @@ module Gitlab
         [storage, relative_path] == [other.storage, other.relative_path]
       end
 
-      def feature_enabled?(flag)
-        @feature_flags.enabled?(flag)
+      def feature_enabled?(flag, on_by_default: false)
+        @feature_flags.enabled?(flag, on_by_default: on_by_default)
       end
 
       def add_branch(branch_name, user:, target:)
diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb
index aab59b120..e2d00636a 100644
--- a/ruby/lib/gitlab/git/repository_mirroring.rb
+++ b/ruby/lib/gitlab/git/repository_mirroring.rb
@@ -13,7 +13,7 @@ module Gitlab
       }.freeze
 
       def remote_branches(remote_name, env:)
-        if feature_enabled?(:remote_branches_ls_remote)
+        if feature_enabled?(:remote_branches_ls_remote, on_by_default: true)
           experimental_remote_branches(remote_name, env: env)
         else
           branches = []
diff --git a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
index dca981717..275504270 100644
--- a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
+++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb
@@ -28,7 +28,7 @@ describe Gitlab::Git::RepositoryMirroring do
       env = { option_a: true, option_b: false }
 
       allow(repository).to receive(:feature_enabled?)
-        .with(:remote_branches_ls_remote)
+        .with(:remote_branches_ls_remote, on_by_default: true)
         .and_return(true)
       expect(repository).to receive(:list_remote_refs)
         .with('remote_a', env: env)