diff options
author | Shinya Maeda <shinya@gitlab.com> | 2019-04-24 18:37:29 +0700 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2019-04-30 21:15:39 +0700 |
commit | daa8f784d016091fd2a56fd195cbd6da2f199350 (patch) | |
tree | 0176ed1b51776ca2224b4309ee180d269c9b0453 | |
parent | a96e96d5c8829348fed969d00395be93290577d4 (diff) | |
download | gitlab-ce-daa8f784d016091fd2a56fd195cbd6da2f199350.tar.gz |
Fix environment automatic on_stop trigger
Due to the nature of pipelines for merge requests, deployments.ref can
be a merge request ref instead of a branch name.
We support the environment auto-stop hook for this case
-rw-r--r-- | app/models/merge_request.rb | 10 | ||||
-rw-r--r-- | app/services/ci/stop_environments_service.rb | 16 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 5 | ||||
-rw-r--r-- | app/services/merge_requests/close_service.rb | 1 | ||||
-rw-r--r-- | app/services/merge_requests/post_merge_service.rb | 1 | ||||
-rw-r--r-- | changelogs/unreleased/fix-environment-on-stop-not-work.yml | 5 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 44 | ||||
-rw-r--r-- | spec/services/ci/stop_environments_service_spec.rb | 76 | ||||
-rw-r--r-- | spec/services/merge_requests/close_service_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/merge_requests/post_merge_service_spec.rb | 8 |
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 |