Commit c5133c23 authored by Grzegorz Bizon's avatar Grzegorz Bizon

Merge branch '29651-fix-triggered-pipelines-depth' into 'master'

Fix error when third level trigger pipeline

See merge request gitlab-org/gitlab!42192
parents c1e1a61c f5b44416
...@@ -823,7 +823,9 @@ module Ci ...@@ -823,7 +823,9 @@ module Ci
def same_family_pipeline_ids def same_family_pipeline_ids
if ::Gitlab::Ci::Features.child_of_child_pipeline_enabled?(project) if ::Gitlab::Ci::Features.child_of_child_pipeline_enabled?(project)
::Gitlab::Ci::PipelineObjectHierarchy.new(base_and_ancestors).base_and_descendants.select(:id) ::Gitlab::Ci::PipelineObjectHierarchy.new(
base_and_ancestors(same_project: true), options: { same_project: true }
).base_and_descendants.select(:id)
else else
# If pipeline is a child of another pipeline, include the parent # If pipeline is a child of another pipeline, include the parent
# and the siblings, otherwise return only itself and children. # and the siblings, otherwise return only itself and children.
...@@ -1062,11 +1064,11 @@ module Ci ...@@ -1062,11 +1064,11 @@ module Ci
self.ci_ref = Ci::Ref.ensure_for(self) self.ci_ref = Ci::Ref.ensure_for(self)
end end
def base_and_ancestors def base_and_ancestors(same_project: false)
# Without using `unscoped`, caller scope is also included into the query. # Without using `unscoped`, caller scope is also included into the query.
# Using `unscoped` here will be redundant after Rails 6.1 # Using `unscoped` here will be redundant after Rails 6.1
::Gitlab::Ci::PipelineObjectHierarchy ::Gitlab::Ci::PipelineObjectHierarchy
.new(self.class.unscoped.where(id: id)) .new(self.class.unscoped.where(id: id), options: { same_project: same_project })
.base_and_ancestors .base_and_ancestors
end end
......
...@@ -121,7 +121,7 @@ module Ci ...@@ -121,7 +121,7 @@ module Ci
def has_max_descendants_depth? def has_max_descendants_depth?
return false unless @bridge.triggers_child_pipeline? return false unless @bridge.triggers_child_pipeline?
ancestors_of_new_child = @bridge.pipeline.base_and_ancestors ancestors_of_new_child = @bridge.pipeline.base_and_ancestors(same_project: true)
ancestors_of_new_child.count > MAX_DESCENDANTS_DEPTH ancestors_of_new_child.count > MAX_DESCENDANTS_DEPTH
end end
end end
......
---
title: Fix error when third level trigger pipeline
merge_request: 42192
author:
type: fixed
...@@ -20,14 +20,26 @@ module Gitlab ...@@ -20,14 +20,26 @@ module Gitlab
def ancestor_conditions(cte) def ancestor_conditions(cte)
middle_table[:source_pipeline_id].eq(objects_table[:id]).and( middle_table[:source_pipeline_id].eq(objects_table[:id]).and(
middle_table[:pipeline_id].eq(cte.table[:id]) middle_table[:pipeline_id].eq(cte.table[:id])
).and(
same_project_condition
) )
end end
def descendant_conditions(cte) def descendant_conditions(cte)
middle_table[:pipeline_id].eq(objects_table[:id]).and( middle_table[:pipeline_id].eq(objects_table[:id]).and(
middle_table[:source_pipeline_id].eq(cte.table[:id]) middle_table[:source_pipeline_id].eq(cte.table[:id])
).and(
same_project_condition
) )
end end
def same_project_condition
if options[:same_project]
middle_table[:source_project_id].eq(middle_table[:project_id])
else
Arel.sql('TRUE')
end
end
end end
end end
end end
...@@ -7,18 +7,19 @@ module Gitlab ...@@ -7,18 +7,19 @@ module Gitlab
class ObjectHierarchy class ObjectHierarchy
DEPTH_COLUMN = :depth DEPTH_COLUMN = :depth
attr_reader :ancestors_base, :descendants_base, :model attr_reader :ancestors_base, :descendants_base, :model, :options
# ancestors_base - An instance of ActiveRecord::Relation for which to # ancestors_base - An instance of ActiveRecord::Relation for which to
# get parent objects. # get parent objects.
# descendants_base - An instance of ActiveRecord::Relation for which to # descendants_base - An instance of ActiveRecord::Relation for which to
# get child objects. If omitted, ancestors_base is used. # get child objects. If omitted, ancestors_base is used.
def initialize(ancestors_base, descendants_base = ancestors_base) def initialize(ancestors_base, descendants_base = ancestors_base, options: {})
raise ArgumentError.new("Model of ancestors_base does not match model of descendants_base") if ancestors_base.model != descendants_base.model raise ArgumentError.new("Model of ancestors_base does not match model of descendants_base") if ancestors_base.model != descendants_base.model
@ancestors_base = ancestors_base @ancestors_base = ancestors_base
@descendants_base = descendants_base @descendants_base = descendants_base
@model = ancestors_base.model @model = ancestors_base.model
@options = options
end end
# Returns the set of descendants of a given relation, but excluding the given # Returns the set of descendants of a given relation, but excluding the given
......
...@@ -3,37 +3,43 @@ ...@@ -3,37 +3,43 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
let_it_be(:project) { create_default(:project, :repository) } include Ci::SourcePipelineHelpers
let_it_be(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) }
let_it_be(:ancestor) { create(:ci_pipeline, project: pipeline.project) } let_it_be(:project) { create(:project, :repository) }
let_it_be(:parent) { create(:ci_pipeline, project: pipeline.project) } let_it_be(:ancestor) { create(:ci_pipeline, project: project) }
let_it_be(:child) { create(:ci_pipeline, project: pipeline.project) } let_it_be(:parent) { create(:ci_pipeline, project: project) }
let_it_be(:cousin_parent) { create(:ci_pipeline, project: pipeline.project) } let_it_be(:child) { create(:ci_pipeline, project: project) }
let_it_be(:cousin) { create(:ci_pipeline, project: pipeline.project) } let_it_be(:cousin_parent) { create(:ci_pipeline, project: project) }
let_it_be(:cousin) { create(:ci_pipeline, project: project) }
let_it_be(:triggered_pipeline) { create(:ci_pipeline) }
before_all do before_all do
create_source_relation(ancestor, parent) create_source_pipeline(ancestor, parent)
create_source_relation(ancestor, cousin_parent) create_source_pipeline(ancestor, cousin_parent)
create_source_relation(parent, child) create_source_pipeline(parent, child)
create_source_relation(cousin_parent, cousin) create_source_pipeline(cousin_parent, cousin)
create_source_pipeline(child, triggered_pipeline)
end end
describe '#base_and_ancestors' do describe '#base_and_ancestors' do
it 'includes the base and its ancestors' do it 'includes the base and its ancestors' do
relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).base_and_ancestors relation = described_class.new(::Ci::Pipeline.where(id: parent.id),
options: { same_project: true }).base_and_ancestors
expect(relation).to contain_exactly(ancestor, parent) expect(relation).to contain_exactly(ancestor, parent)
end end
it 'can find ancestors upto a certain level' do it 'can find ancestors upto a certain level' do
relation = described_class.new(::Ci::Pipeline.where(id: child.id)).base_and_ancestors(upto: ancestor.id) relation = described_class.new(::Ci::Pipeline.where(id: child.id),
options: { same_project: true }).base_and_ancestors(upto: ancestor.id)
expect(relation).to contain_exactly(parent, child) expect(relation).to contain_exactly(parent, child)
end end
describe 'hierarchy_order option' do describe 'hierarchy_order option' do
let(:relation) do let(:relation) do
described_class.new(::Ci::Pipeline.where(id: child.id)).base_and_ancestors(hierarchy_order: hierarchy_order) described_class.new(::Ci::Pipeline.where(id: child.id),
options: { same_project: true }).base_and_ancestors(hierarchy_order: hierarchy_order)
end end
context ':asc' do context ':asc' do
...@@ -56,14 +62,16 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do ...@@ -56,14 +62,16 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
describe '#base_and_descendants' do describe '#base_and_descendants' do
it 'includes the base and its descendants' do it 'includes the base and its descendants' do
relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).base_and_descendants relation = described_class.new(::Ci::Pipeline.where(id: parent.id),
options: { same_project: true }).base_and_descendants
expect(relation).to contain_exactly(parent, child) expect(relation).to contain_exactly(parent, child)
end end
context 'when with_depth is true' do context 'when with_depth is true' do
let(:relation) do let(:relation) do
described_class.new(::Ci::Pipeline.where(id: ancestor.id)).base_and_descendants(with_depth: true) described_class.new(::Ci::Pipeline.where(id: ancestor.id),
options: { same_project: true }).base_and_descendants(with_depth: true)
end end
it 'includes depth in the results' do it 'includes depth in the results' do
...@@ -84,7 +92,8 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do ...@@ -84,7 +92,8 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
describe '#all_objects' do describe '#all_objects' do
it 'includes its ancestors and descendants' do it 'includes its ancestors and descendants' do
relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).all_objects relation = described_class.new(::Ci::Pipeline.where(id: parent.id),
options: { same_project: true }).all_objects
expect(relation).to contain_exactly(ancestor, parent, child) expect(relation).to contain_exactly(ancestor, parent, child)
end end
...@@ -92,20 +101,11 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do ...@@ -92,20 +101,11 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
it 'returns all family tree' do it 'returns all family tree' do
relation = described_class.new( relation = described_class.new(
::Ci::Pipeline.where(id: child.id), ::Ci::Pipeline.where(id: child.id),
described_class.new(::Ci::Pipeline.where(id: child.id)).base_and_ancestors described_class.new(::Ci::Pipeline.where(id: child.id), options: { same_project: true }).base_and_ancestors,
options: { same_project: true }
).all_objects ).all_objects
expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin) expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin)
end end
end end
private
def create_source_relation(parent, child)
create(:ci_sources_pipeline,
source_job: create(:ci_build, pipeline: parent),
source_project: parent.project,
pipeline: child,
project: child.project)
end
end end
...@@ -5,6 +5,7 @@ require 'spec_helper' ...@@ -5,6 +5,7 @@ require 'spec_helper'
RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
include ProjectForksHelper include ProjectForksHelper
include StubRequests include StubRequests
include Ci::SourcePipelineHelpers
let_it_be(:user) { create(:user) } let_it_be(:user) { create(:user) }
let_it_be(:namespace) { create_default(:namespace) } let_it_be(:namespace) { create_default(:namespace) }
...@@ -2629,17 +2630,8 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -2629,17 +2630,8 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
let(:sibling) { create(:ci_pipeline, project: pipeline.project) } let(:sibling) { create(:ci_pipeline, project: pipeline.project) }
before do before do
create(:ci_sources_pipeline, create_source_pipeline(parent, pipeline)
source_job: create(:ci_build, pipeline: parent), create_source_pipeline(parent, sibling)
source_project: parent.project,
pipeline: pipeline,
project: pipeline.project)
create(:ci_sources_pipeline,
source_job: create(:ci_build, pipeline: parent),
source_project: parent.project,
pipeline: sibling,
project: sibling.project)
end end
it 'returns parent sibling and self ids' do it 'returns parent sibling and self ids' do
...@@ -2651,11 +2643,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -2651,11 +2643,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
let(:child) { create(:ci_pipeline, project: pipeline.project) } let(:child) { create(:ci_pipeline, project: pipeline.project) }
before do before do
create(:ci_sources_pipeline, create_source_pipeline(pipeline, child)
source_job: create(:ci_build, pipeline: pipeline),
source_project: pipeline.project,
pipeline: child,
project: child.project)
end end
it 'returns self and child ids' do it 'returns self and child ids' do
...@@ -2670,29 +2658,10 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -2670,29 +2658,10 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
let(:cousin) { create(:ci_pipeline, project: pipeline.project) } let(:cousin) { create(:ci_pipeline, project: pipeline.project) }
before do before do
create(:ci_sources_pipeline, create_source_pipeline(ancestor, parent)
source_job: create(:ci_build, pipeline: ancestor), create_source_pipeline(ancestor, cousin_parent)
source_project: ancestor.project, create_source_pipeline(parent, pipeline)
pipeline: parent, create_source_pipeline(cousin_parent, cousin)
project: parent.project)
create(:ci_sources_pipeline,
source_job: create(:ci_build, pipeline: ancestor),
source_project: ancestor.project,
pipeline: cousin_parent,
project: cousin_parent.project)
create(:ci_sources_pipeline,
source_job: create(:ci_build, pipeline: parent),
source_project: parent.project,
pipeline: pipeline,
project: pipeline.project)
create(:ci_sources_pipeline,
source_job: create(:ci_build, pipeline: cousin_parent),
source_project: cousin_parent.project,
pipeline: cousin,
project: cousin.project)
end end
it 'returns all family ids' do it 'returns all family ids' do
...@@ -2701,6 +2670,18 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -2701,6 +2670,18 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
) )
end end
end end
context 'when pipeline is a triggered pipeline' do
let(:upstream) { create(:ci_pipeline, project: create(:project)) }
before do
create_source_pipeline(upstream, pipeline)
end
it 'returns self id' do
expect(subject).to contain_exactly(pipeline.id)
end
end
end end
describe '#stuck?' do describe '#stuck?' do
...@@ -3556,4 +3537,78 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do ...@@ -3556,4 +3537,78 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
expect(builds).not_to include(karma) expect(builds).not_to include(karma)
end end
end end
describe '#base_and_ancestors' do
let(:same_project) { false }
subject { pipeline.base_and_ancestors(same_project: same_project) }
context 'when pipeline is not child nor parent' do
it 'returns just the pipeline itself' do
expect(subject).to contain_exactly(pipeline)
end
end
context 'when pipeline is child' do
let(:parent) { create(:ci_pipeline, project: pipeline.project) }
let(:sibling) { create(:ci_pipeline, project: pipeline.project) }
before do
create_source_pipeline(parent, pipeline)
create_source_pipeline(parent, sibling)
end
it 'returns parent and self' do
expect(subject).to contain_exactly(parent, pipeline)
end
end
context 'when pipeline is parent' do
let(:child) { create(:ci_pipeline, project: pipeline.project) }
before do
create_source_pipeline(pipeline, child)
end
it 'returns self' do
expect(subject).to contain_exactly(pipeline)
end
end
context 'when pipeline is a child of a child pipeline' do
let(:ancestor) { create(:ci_pipeline, project: pipeline.project) }
let(:parent) { create(:ci_pipeline, project: pipeline.project) }
before do
create_source_pipeline(ancestor, parent)
create_source_pipeline(parent, pipeline)
end
it 'returns self, parent and ancestor' do
expect(subject).to contain_exactly(ancestor, parent, pipeline)
end
end
context 'when pipeline is a triggered pipeline' do
let(:upstream) { create(:ci_pipeline, project: create(:project)) }
before do
create_source_pipeline(upstream, pipeline)
end
context 'same_project: false' do
it 'returns upstream and self' do
expect(subject).to contain_exactly(pipeline, upstream)
end
end
context 'same_project: true' do
let(:same_project) { true }
it 'returns self' do
expect(subject).to contain_exactly(pipeline)
end
end
end
end
end end
...@@ -179,7 +179,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do ...@@ -179,7 +179,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
end end
end end
context 'when downstream project is the same as the job project' do context 'when downstream project is the same as the upstream project' do
let(:trigger) do let(:trigger) do
{ trigger: { project: upstream_project.full_path } } { trigger: { project: upstream_project.full_path } }
end end
...@@ -311,18 +311,14 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do ...@@ -311,18 +311,14 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
end end
end end
context 'when upstream pipeline is a first descendant pipeline' do context 'when upstream pipeline has a parent pipeline' do
let!(:pipeline_source) do before do
create(:ci_sources_pipeline, create(:ci_sources_pipeline,
source_pipeline: create(:ci_pipeline, project: upstream_pipeline.project), source_pipeline: create(:ci_pipeline, project: upstream_pipeline.project),
pipeline: upstream_pipeline pipeline: upstream_pipeline
) )
end end
before do
upstream_pipeline.update!(source: :parent_pipeline)
end
it 'creates the pipeline' do it 'creates the pipeline' do
expect { service.execute(bridge) } expect { service.execute(bridge) }
.to change { Ci::Pipeline.count }.by(1) .to change { Ci::Pipeline.count }.by(1)
...@@ -345,8 +341,8 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do ...@@ -345,8 +341,8 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
end end
end end
context 'when upstream pipeline is a second descendant pipeline' do context 'when upstream pipeline has a parent pipeline, which has a parent pipeline' do
let!(:pipeline_source) do before do
parent_of_upstream_pipeline = create(:ci_pipeline, project: upstream_pipeline.project) parent_of_upstream_pipeline = create(:ci_pipeline, project: upstream_pipeline.project)
create(:ci_sources_pipeline, create(:ci_sources_pipeline,
...@@ -360,10 +356,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do ...@@ -360,10 +356,6 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
) )
end end
before do
upstream_pipeline.update!(source: :parent_pipeline)
end
it 'does not create a second descendant pipeline' do it 'does not create a second descendant pipeline' do
expect { service.execute(bridge) } expect { service.execute(bridge) }
.not_to change { Ci::Pipeline.count } .not_to change { Ci::Pipeline.count }
...@@ -372,6 +364,27 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do ...@@ -372,6 +364,27 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
expect(bridge.failure_reason).to eq 'reached_max_descendant_pipelines_depth' expect(bridge.failure_reason).to eq 'reached_max_descendant_pipelines_depth'
end end
end end
context 'when upstream pipeline has two level upstream pipelines from different projects' do
before do
upstream_of_upstream_of_upstream_pipeline = create(:ci_pipeline)
upstream_of_upstream_pipeline = create(:ci_pipeline)
create(:ci_sources_pipeline,
source_pipeline: upstream_of_upstream_of_upstream_pipeline,
pipeline: upstream_of_upstream_pipeline
)
create(:ci_sources_pipeline,
source_pipeline: upstream_of_upstream_pipeline,
pipeline: upstream_pipeline
)
end
it 'create the pipeline' do
expect { service.execute(bridge) }.to change { Ci::Pipeline.count }.by(1)
end
end
end end
end end
......
# frozen_string_literal: true
module Ci
module SourcePipelineHelpers
def create_source_pipeline(upstream, downstream)
create(:ci_sources_pipeline,
source_job: create(:ci_build, pipeline: upstream),
source_project: upstream.project,
pipeline: downstream,
project: downstream.project)
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