Merge branch 'stop-exposing-mr-refs-in-favor-of-persistent-refs' into 'master'

Stop exposing MR refs in favor of persistent pipeline refs

Closes #35918 and #35140

See merge request gitlab-org/gitlab!22198

app/presenters/ci/build_runner_presenter.rb

@@ -34,7 +34,7 @@ module Ci
 
     def refspecs
       specs = []
-      specs << refspec_for_pipeline_ref if merge_request_ref?
+      specs << refspec_for_pipeline_ref if should_expose_merge_request_ref?
       specs << refspec_for_persistent_ref if persistent_ref_exist?
 
       if git_depth > 0
@@ -50,6 +50,19 @@ module Ci
 
     private
 
+    # We will stop exposing merge request refs when we fully depend on persistent refs
+    # (i.e. remove `refspec_for_pipeline_ref` when we remove `depend_on_persistent_pipeline_ref` feature flag.)
+    # `ci_force_exposing_merge_request_refs` is an extra feature flag that allows us to
+    # forcibly expose MR refs even if the `depend_on_persistent_pipeline_ref` feature flag enabled.
+    # This is useful when we see an unexpected behaviors/reports from users.
+    # See https://gitlab.com/gitlab-org/gitlab/issues/35140.
+    def should_expose_merge_request_ref?
+      return false unless merge_request_ref?
+      return true if Feature.enabled?(:ci_force_exposing_merge_request_refs, project)
+
+      Feature.disabled?(:depend_on_persistent_pipeline_ref, project, default_enabled: true)
+    end
+
     def create_archive(artifacts)
       return unless artifacts[:untracked] || artifacts[:paths]
 

changelogs/unreleased/stop-exposing-mr-refs-in-favor-of-persistent-refs.yml

+---
+title: Stop exposing MR refs in favor of persistent pipeline refs
+merge_request: 22198
+author:
+type: fixed

spec/presenters/ci/build_runner_presenter_spec.rb

@@ -183,29 +183,81 @@ describe Ci::BuildRunnerPresenter do
       let(:pipeline) { merge_request.all_pipelines.first }
       let(:build) { create(:ci_build, ref: pipeline.ref, pipeline: pipeline) }
 
-      it 'returns the correct refspecs' do
-        is_expected
-          .to contain_exactly('+refs/merge-requests/1/head:refs/merge-requests/1/head')
-      end
-
-      context 'when GIT_DEPTH is zero' do
+      context 'when depend_on_persistent_pipeline_ref feature flag is enabled' do
         before do
-          create(:ci_pipeline_variable, key: 'GIT_DEPTH', value: 0, pipeline: build.pipeline)
+          stub_feature_flags(ci_force_exposing_merge_request_refs: false)
+          pipeline.persistent_ref.create
         end
 
         it 'returns the correct refspecs' do
           is_expected
-            .to contain_exactly('+refs/merge-requests/1/head:refs/merge-requests/1/head',
-                                '+refs/heads/*:refs/remotes/origin/*',
-                                '+refs/tags/*:refs/tags/*')
+            .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}")
+        end
+
+        context 'when ci_force_exposing_merge_request_refs feature flag is enabled' do
+          before do
+            stub_feature_flags(ci_force_exposing_merge_request_refs: true)
+          end
+
+          it 'returns the correct refspecs' do
+            is_expected
+              .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+                                  '+refs/merge-requests/1/head:refs/merge-requests/1/head')
+          end
+        end
+
+        context 'when GIT_DEPTH is zero' do
+          before do
+            create(:ci_pipeline_variable, key: 'GIT_DEPTH', value: 0, pipeline: build.pipeline)
+          end
+
+          it 'returns the correct refspecs' do
+            is_expected
+              .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+                                  '+refs/heads/*:refs/remotes/origin/*',
+                                  '+refs/tags/*:refs/tags/*')
+          end
+        end
+
+        context 'when pipeline is legacy detached merge request pipeline' do
+          let(:merge_request) { create(:merge_request, :with_legacy_detached_merge_request_pipeline) }
+
+          it 'returns the correct refspecs' do
+            is_expected.to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+                                           "+refs/heads/#{build.ref}:refs/remotes/origin/#{build.ref}")
+          end
         end
       end
 
-      context 'when pipeline is legacy detached merge request pipeline' do
-        let(:merge_request) { create(:merge_request, :with_legacy_detached_merge_request_pipeline) }
+      context 'when depend_on_persistent_pipeline_ref feature flag is disabled' do
+        before do
+          stub_feature_flags(depend_on_persistent_pipeline_ref: false)
+        end
 
         it 'returns the correct refspecs' do
-          is_expected.to contain_exactly("+refs/heads/#{build.ref}:refs/remotes/origin/#{build.ref}")
+          is_expected
+            .to contain_exactly('+refs/merge-requests/1/head:refs/merge-requests/1/head')
+        end
+
+        context 'when GIT_DEPTH is zero' do
+          before do
+            create(:ci_pipeline_variable, key: 'GIT_DEPTH', value: 0, pipeline: build.pipeline)
+          end
+
+          it 'returns the correct refspecs' do
+            is_expected
+              .to contain_exactly('+refs/merge-requests/1/head:refs/merge-requests/1/head',
+                                  '+refs/heads/*:refs/remotes/origin/*',
+                                  '+refs/tags/*:refs/tags/*')
+          end
+        end
+
+        context 'when pipeline is legacy detached merge request pipeline' do
+          let(:merge_request) { create(:merge_request, :with_legacy_detached_merge_request_pipeline) }
+
+          it 'returns the correct refspecs' do
+            is_expected.to contain_exactly("+refs/heads/#{build.ref}:refs/remotes/origin/#{build.ref}")
+          end
         end
       end
     end