summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2019-04-30 15:13:24 +0000
committerSean McGivern <sean@gitlab.com>2019-04-30 15:13:24 +0000
commit2432a540cff461c5d9c0346dd4021229078d674d (patch)
tree868dc925eaa0a328d00e88f98f4921394d151602
parent191152104399cad8b01d06712501250688fca9d7 (diff)
parentdaa8f784d016091fd2a56fd195cbd6da2f199350 (diff)
downloadgitlab-ce-2432a540cff461c5d9c0346dd4021229078d674d.tar.gz
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
-rw-r--r--app/models/merge_request.rb10
-rw-r--r--app/services/ci/stop_environments_service.rb16
-rw-r--r--app/services/merge_requests/base_service.rb5
-rw-r--r--app/services/merge_requests/close_service.rb1
-rw-r--r--app/services/merge_requests/post_merge_service.rb1
-rw-r--r--changelogs/unreleased/fix-environment-on-stop-not-work.yml5
-rw-r--r--spec/models/merge_request_spec.rb44
-rw-r--r--spec/services/ci/stop_environments_service_spec.rb76
-rw-r--r--spec/services/merge_requests/close_service_spec.rb8
-rw-r--r--spec/services/merge_requests/post_merge_service_spec.rb8
10 files changed, 169 insertions, 5 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index a5b62659b24..c2a1487fc6e 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1054,6 +1054,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"
diff --git a/app/services/ci/stop_environments_service.rb b/app/services/ci/stop_environments_service.rb
index 973ae5ce5aa..d9a800791f2 100644
--- a/app/services/ci/stop_environments_service.rb
+++ b/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
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index b8334a87f6d..a9dd26c02ad 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/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)
diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb
index 04527bb9713..e77051bb1c9 100644
--- a/app/services/merge_requests/close_service.rb
+++ b/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
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index f26e3bee06f..c13f7dd5088 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/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
diff --git a/changelogs/unreleased/fix-environment-on-stop-not-work.yml b/changelogs/unreleased/fix-environment-on-stop-not-work.yml
new file mode 100644
index 00000000000..72e58b26c4d
--- /dev/null
+++ b/changelogs/unreleased/fix-environment-on-stop-not-work.yml
@@ -0,0 +1,5 @@
+---
+title: "`on_stop` is not automatically triggered with pipelines for merge requests"
+merge_request: 27618
+author:
+type: fixed
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index f61857ea5ff..fb7f43b25cf 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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)
diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb
index 31b8d540356..890fa5bc009 100644
--- a/spec/services/ci/stop_environments_service_spec.rb
+++ b/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!)
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb
index aa7dfda4950..ffa612cf315 100644
--- a/spec/services/merge_requests/close_service_spec.rb
+++ b/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
diff --git a/spec/services/merge_requests/post_merge_service_spec.rb b/spec/services/merge_requests/post_merge_service_spec.rb
index 7b87913ab8b..ffc86f68469 100644
--- a/spec/services/merge_requests/post_merge_service_spec.rb
+++ b/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