Commit 9db97edd authored by Thong Kuah's avatar Thong Kuah

Merge branch 'fix-merge-request-environment-relation-follow-up' into 'master'

Fix Merge Request accidentally stops unrelated environments

See merge request gitlab-org/gitlab!84557
parents 4387c3fe c2763b62
...@@ -959,7 +959,7 @@ module Ci ...@@ -959,7 +959,7 @@ module Ci
Ci::Build.latest.where(pipeline: self_and_descendants) Ci::Build.latest.where(pipeline: self_and_descendants)
end end
def environments_in_self_and_descendants def environments_in_self_and_descendants(deployment_status: nil)
# We limit to 100 unique environments for application safety. # We limit to 100 unique environments for application safety.
# See: https://gitlab.com/gitlab-org/gitlab/-/issues/340781#note_699114700 # See: https://gitlab.com/gitlab-org/gitlab/-/issues/340781#note_699114700
expanded_environment_names = expanded_environment_names =
...@@ -969,7 +969,7 @@ module Ci ...@@ -969,7 +969,7 @@ module Ci
.limit(100) .limit(100)
.pluck(:expanded_environment_name) .pluck(:expanded_environment_name)
Environment.where(project: project, name: expanded_environment_names).with_deployment(sha) Environment.where(project: project, name: expanded_environment_names).with_deployment(sha, status: deployment_status)
end end
# With multi-project and parent-child pipelines # With multi-project and parent-child pipelines
......
...@@ -89,13 +89,19 @@ class Environment < ApplicationRecord ...@@ -89,13 +89,19 @@ class Environment < ApplicationRecord
scope :for_project, -> (project) { where(project_id: project) } scope :for_project, -> (project) { where(project_id: project) }
scope :for_tier, -> (tier) { where(tier: tier).where.not(tier: nil) } scope :for_tier, -> (tier) { where(tier: tier).where.not(tier: nil) }
scope :with_deployment, -> (sha) { where('EXISTS (?)', Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha)) }
scope :unfoldered, -> { where(environment_type: nil) } scope :unfoldered, -> { where(environment_type: nil) }
scope :with_rank, -> do scope :with_rank, -> do
select('environments.*, rank() OVER (PARTITION BY project_id ORDER BY id DESC)') select('environments.*, rank() OVER (PARTITION BY project_id ORDER BY id DESC)')
end end
scope :for_id, -> (id) { where(id: id) } scope :for_id, -> (id) { where(id: id) }
scope :with_deployment, -> (sha, status: nil) do
deployments = Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha)
deployments = deployments.where(status: status) if status
where('EXISTS (?)', deployments)
end
scope :stopped_review_apps, -> (before, limit) do scope :stopped_review_apps, -> (before, limit) do
stopped stopped
.in_review_folder .in_review_folder
......
...@@ -1456,9 +1456,9 @@ class MergeRequest < ApplicationRecord ...@@ -1456,9 +1456,9 @@ class MergeRequest < ApplicationRecord
Environment.where(project: project, name: environments) Environment.where(project: project, name: environments)
end end
def environments_in_head_pipeline def environments_in_head_pipeline(deployment_status: nil)
if ::Feature.enabled?(:fix_related_environments_for_merge_requests, target_project, default_enabled: :yaml) if ::Feature.enabled?(:fix_related_environments_for_merge_requests, target_project, default_enabled: :yaml)
actual_head_pipeline&.environments_in_self_and_descendants || Environment.none actual_head_pipeline&.environments_in_self_and_descendants(deployment_status: deployment_status) || Environment.none
else else
legacy_environments legacy_environments
end end
......
...@@ -19,7 +19,9 @@ module Environments ...@@ -19,7 +19,9 @@ module Environments
end end
def execute_for_merge_request(merge_request) def execute_for_merge_request(merge_request)
merge_request.environments_in_head_pipeline.each { |environment| execute(environment) } merge_request.environments_in_head_pipeline(deployment_status: :success).each do |environment|
execute(environment)
end
end end
private private
......
...@@ -189,6 +189,20 @@ FactoryBot.define do ...@@ -189,6 +189,20 @@ FactoryBot.define do
set_expanded_environment_name set_expanded_environment_name
end end
trait :start_staging do
name { 'start staging' }
environment { 'staging' }
options do
{
script: %w(ls),
environment: { name: 'staging', action: 'start' }
}
end
set_expanded_environment_name
end
trait :stop_staging do trait :stop_staging do
name { 'stop staging' } name { 'stop staging' }
environment { 'staging' } environment { 'staging' }
......
...@@ -349,15 +349,28 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do ...@@ -349,15 +349,28 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
end end
describe '.with_deployment' do describe '.with_deployment' do
subject { described_class.with_deployment(sha) } subject { described_class.with_deployment(sha, status: status) }
let(:environment) { create(:environment, project: project) } let(:environment) { create(:environment, project: project) }
let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } let(:sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
let(:status) { nil }
context 'when deployment has the specified sha' do context 'when deployment has the specified sha' do
let!(:deployment) { create(:deployment, environment: environment, sha: sha) } let!(:deployment) { create(:deployment, environment: environment, sha: sha) }
it { is_expected.to eq([environment]) } it { is_expected.to eq([environment]) }
context 'with success status filter' do
let(:status) { :success }
it { is_expected.to be_empty }
end
context 'with created status filter' do
let(:status) { :created }
it { is_expected.to contain_exactly(environment) }
end
end end
context 'when deployment does not have the specified sha' do context 'when deployment does not have the specified sha' do
......
...@@ -202,6 +202,7 @@ RSpec.describe Environments::StopService do ...@@ -202,6 +202,7 @@ RSpec.describe Environments::StopService do
context 'with environment related jobs ' do context 'with environment related jobs ' do
let!(:environment) { create(:environment, :available, name: 'staging', project: project) } let!(:environment) { create(:environment, :available, name: 'staging', project: project) }
let!(:prepare_staging_job) { create(:ci_build, :prepare_staging, pipeline: pipeline, project: project) } let!(:prepare_staging_job) { create(:ci_build, :prepare_staging, pipeline: pipeline, project: project) }
let!(:start_staging_job) { create(:ci_build, :start_staging, :with_deployment, :manual, pipeline: pipeline, project: project) }
let!(:stop_staging_job) { create(:ci_build, :stop_staging, :manual, pipeline: pipeline, project: project) } let!(:stop_staging_job) { create(:ci_build, :stop_staging, :manual, pipeline: pipeline, project: project) }
it 'does not stop environments that was not started by the merge request' do it 'does not stop environments that was not started by the merge request' do
......
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