Commit 2432a540 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'fix-environment-on-stop-not-work' into 'master'

`on_stop` is not automatically triggered with pipelines for merge requests

Closes #60885

See merge request gitlab-org/gitlab-ce!27618
parents 19115210 daa8f784
...@@ -1054,6 +1054,16 @@ class MergeRequest < ApplicationRecord ...@@ -1054,6 +1054,16 @@ class MergeRequest < ApplicationRecord
@environments[current_user] @environments[current_user]
end 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 def state_human_name
if merged? if merged?
"Merged" "Merged"
......
...@@ -9,12 +9,11 @@ module Ci ...@@ -9,12 +9,11 @@ module Ci
return unless @ref.present? return unless @ref.present?
environments.each do |environment| environments.each { |environment| stop(environment) }
next unless environment.stop_action_available?
next unless can?(current_user, :stop_environment, environment)
environment.stop_with_action!(current_user)
end end
def execute_for_merge_request(merge_request)
merge_request.environments.each { |environment| stop(environment) }
end end
private private
...@@ -24,5 +23,12 @@ module Ci ...@@ -24,5 +23,12 @@ module Ci
.new(project, current_user, ref: @ref, recently_updated: true) .new(project, current_user, ref: @ref, recently_updated: true)
.execute .execute
end 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
end end
...@@ -24,6 +24,11 @@ module MergeRequests ...@@ -24,6 +24,11 @@ module MergeRequests
end end
end end
def cleanup_environments(merge_request)
Ci::StopEnvironmentsService.new(merge_request.source_project, current_user)
.execute_for_merge_request(merge_request)
end
private private
def handle_wip_event(merge_request) def handle_wip_event(merge_request)
......
...@@ -17,6 +17,7 @@ module MergeRequests ...@@ -17,6 +17,7 @@ module MergeRequests
execute_hooks(merge_request, 'close') execute_hooks(merge_request, 'close')
invalidate_cache_counts(merge_request, users: merge_request.assignees) invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches merge_request.update_project_counter_caches
cleanup_environments(merge_request)
end end
merge_request merge_request
......
...@@ -18,6 +18,7 @@ module MergeRequests ...@@ -18,6 +18,7 @@ module MergeRequests
invalidate_cache_counts(merge_request, users: merge_request.assignees) invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches merge_request.update_project_counter_caches
delete_non_latest_diffs(merge_request) delete_non_latest_diffs(merge_request)
cleanup_environments(merge_request)
end end
private private
......
---
title: "`on_stop` is not automatically triggered with pipelines for merge requests"
merge_request: 27618
author:
type: fixed
...@@ -2262,6 +2262,50 @@ describe MergeRequest do ...@@ -2262,6 +2262,50 @@ describe MergeRequest do
end end
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 describe "#reload_diff" do
it 'calls MergeRequests::ReloadDiffsService#execute with correct params' do it 'calls MergeRequests::ReloadDiffsService#execute with correct params' do
user = create(:user) user = create(:user)
......
...@@ -105,6 +105,82 @@ describe Ci::StopEnvironmentsService do ...@@ -105,6 +105,82 @@ describe Ci::StopEnvironmentsService do
end end
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) def expect_environment_stopped_on(branch)
expect_any_instance_of(Environment) expect_any_instance_of(Environment)
.to receive(:stop!) .to receive(:stop!)
......
...@@ -74,6 +74,14 @@ describe MergeRequests::CloseService do ...@@ -74,6 +74,14 @@ describe MergeRequests::CloseService do
.to change { project.open_merge_requests_count }.from(1).to(0) .to change { project.open_merge_requests_count }.from(1).to(0)
end 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 context 'current user is not authorized to close merge request' do
before do before do
perform_enqueued_jobs do perform_enqueued_jobs do
......
...@@ -62,5 +62,13 @@ describe MergeRequests::PostMergeService do ...@@ -62,5 +62,13 @@ describe MergeRequests::PostMergeService do
expect(merge_request.reload).to be_merged expect(merge_request.reload).to be_merged
end 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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment