Automatic merge of gitlab-org/gitlab-ce master

app/models/merge_request.rb

@@ -1056,6 +1056,16 @@ class MergeRequest < ApplicationRecord
     @environments[current_user]
   end
 
+  ##
+  # This method is for looking for active environments which created via pipelines for merge requests.
+  # Since deployments run on a merge request ref (e.g. `refs/merge-requests/:iid/head`),
+  # we cannot look up environments with source branch name.
+  def environments
+    return Environment.none unless actual_head_pipeline&.triggered_by_merge_request?
+
+    actual_head_pipeline.environments
+  end
+
   def state_human_name
     if merged?
       "Merged"

app/services/ci/stop_environments_service.rb

@@ -9,12 +9,11 @@ module Ci
 
       return unless @ref.present?
 
-      environments.each do |environment|
-        next unless environment.stop_action_available?
-        next unless can?(current_user, :stop_environment, environment)
+      environments.each { |environment| stop(environment) }
+    end
 
-        environment.stop_with_action!(current_user)
-      end
+    def execute_for_merge_request(merge_request)
+      merge_request.environments.each { |environment| stop(environment) }
     end
 
     private
@@ -24,5 +23,12 @@ module Ci
         .new(project, current_user, ref: @ref, recently_updated: true)
         .execute
     end
+
+    def stop(environment)
+      return unless environment.stop_action_available?
+      return unless can?(current_user, :stop_environment, environment)
+
+      environment.stop_with_action!(current_user)
+    end
   end
 end

app/services/merge_requests/base_service.rb

@@ -24,6 +24,11 @@ module MergeRequests
       end
     end
 
+    def cleanup_environments(merge_request)
+      Ci::StopEnvironmentsService.new(merge_request.source_project, current_user)
+                                 .execute_for_merge_request(merge_request)
+    end
+
     private
 
     def handle_wip_event(merge_request)

app/services/merge_requests/close_service.rb

@@ -17,6 +17,7 @@ module MergeRequests
         execute_hooks(merge_request, 'close')
         invalidate_cache_counts(merge_request, users: merge_request.assignees)
         merge_request.update_project_counter_caches
+        cleanup_environments(merge_request)
       end
 
       merge_request

app/services/merge_requests/post_merge_service.rb

@@ -18,6 +18,7 @@ module MergeRequests
       invalidate_cache_counts(merge_request, users: merge_request.assignees)
       merge_request.update_project_counter_caches
       delete_non_latest_diffs(merge_request)
+      cleanup_environments(merge_request)
     end
 
     private

changelogs/unreleased/fix-environment-on-stop-not-work.yml

+---
+title: "`on_stop` is not automatically triggered with pipelines for merge requests"
+merge_request: 27618
+author:
+type: fixed

spec/models/merge_request_spec.rb

@@ -2262,6 +2262,50 @@ describe MergeRequest do
     end
   end
 
+  describe "#environments" do
+    subject { merge_request.environments }
+
+    let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') }
+    let(:project) { merge_request.project }
+
+    let(:pipeline) do
+      create(:ci_pipeline,
+        source: :merge_request_event,
+        merge_request: merge_request, project: project,
+        sha: merge_request.diff_head_sha,
+        merge_requests_as_head_pipeline: [merge_request])
+    end
+
+    let!(:job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) }
+
+    it 'returns environments' do
+      is_expected.to eq(pipeline.environments)
+      expect(subject.count).to be(1)
+    end
+
+    context 'when pipeline is not associated with environments' do
+      let!(:job) { create(:ci_build, pipeline: pipeline, project: project) }
+
+      it 'returns empty array' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when pipeline is not a pipeline for merge request' do
+      let(:pipeline) do
+        create(:ci_pipeline,
+          project: project,
+          ref: 'feature',
+          sha: merge_request.diff_head_sha,
+          merge_requests_as_head_pipeline: [merge_request])
+      end
+
+      it 'returns empty relation' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
   describe "#reload_diff" do
     it 'calls MergeRequests::ReloadDiffsService#execute with correct params' do
       user = create(:user)

spec/services/ci/stop_environments_service_spec.rb

@@ -105,6 +105,82 @@ describe Ci::StopEnvironmentsService do
     end
   end
 
+  describe '#execute_for_merge_request' do
+    subject { service.execute_for_merge_request(merge_request) }
+
+    let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master') }
+    let(:project) { merge_request.project }
+    let(:user) { create(:user) }
+
+    let(:pipeline) do
+      create(:ci_pipeline,
+        source: :merge_request_event,
+        merge_request: merge_request,
+        project: project,
+        sha: merge_request.diff_head_sha,
+        merge_requests_as_head_pipeline: [merge_request])
+    end
+
+    let!(:review_job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) }
+    let!(:stop_review_job) { create(:ci_build, :stop_review_app, :manual, pipeline: pipeline, project: project) }
+
+    before do
+      review_job.deployment.success!
+    end
+
+    it 'has active environment at first' do
+      expect(pipeline.environments.first).to be_available
+    end
+
+    context 'when user is a developer' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'stops the active environment' do
+        subject
+
+        expect(pipeline.environments.first).to be_stopped
+      end
+    end
+
+    context 'when user is a reporter' do
+      before do
+        project.add_reporter(user)
+      end
+
+      it 'does not stop the active environment' do
+        subject
+
+        expect(pipeline.environments.first).to be_available
+      end
+    end
+
+    context 'when pipeline is not associated with environments' do
+      let!(:job) { create(:ci_build, pipeline: pipeline, project: project) }
+
+      it 'does not raise exception' do
+        expect { subject }.not_to raise_exception
+      end
+    end
+
+    context 'when pipeline is not a pipeline for merge request' do
+      let(:pipeline) do
+        create(:ci_pipeline,
+          project: project,
+          ref: 'feature',
+          sha: merge_request.diff_head_sha,
+          merge_requests_as_head_pipeline: [merge_request])
+      end
+
+      it 'does not stop the active environment' do
+        subject
+
+        expect(pipeline.environments.first).to be_available
+      end
+    end
+  end
+
   def expect_environment_stopped_on(branch)
     expect_any_instance_of(Environment)
       .to receive(:stop!)

spec/services/merge_requests/close_service_spec.rb

@@ -74,6 +74,14 @@ describe MergeRequests::CloseService do
         .to change { project.open_merge_requests_count }.from(1).to(0)
     end
 
+    it 'clean up environments for the merge request' do
+      expect_next_instance_of(Ci::StopEnvironmentsService) do |service|
+        expect(service).to receive(:execute_for_merge_request).with(merge_request)
+      end
+
+      described_class.new(project, user).execute(merge_request)
+    end
+
     context 'current user is not authorized to close merge request' do
       before do
         perform_enqueued_jobs do

spec/services/merge_requests/post_merge_service_spec.rb

@@ -62,5 +62,13 @@ describe MergeRequests::PostMergeService do
 
       expect(merge_request.reload).to be_merged
     end
+
+    it 'clean up environments for the merge request' do
+      expect_next_instance_of(Ci::StopEnvironmentsService) do |service|
+        expect(service).to receive(:execute_for_merge_request).with(merge_request)
+      end
+
+      described_class.new(project, user).execute(merge_request)
+    end
   end
 end