From 93e951821543b0cbb12807cc710d3e21d7db8993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 13 Aug 2019 13:29:56 +0200 Subject: [PATCH] Require `needs:` to be present This changes the `needs:` logic to require that all jobs to be present. Instead of skipping do fail the pipeline creation if `needs:` dependency is not found. --- lib/gitlab/ci/pipeline/chain/populate.rb | 11 ++-- lib/gitlab/ci/pipeline/seed/base.rb | 4 ++ lib/gitlab/ci/pipeline/seed/build.rb | 54 +++++++++++-------- lib/gitlab/ci/pipeline/seed/stage.rb | 6 +++ .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 17 +++++- .../lib/gitlab/ci/pipeline/seed/stage_spec.rb | 10 ++++ .../ci/create_pipeline_service_spec.rb | 3 +- 7 files changed, 78 insertions(+), 27 deletions(-) diff --git a/lib/gitlab/ci/pipeline/chain/populate.rb b/lib/gitlab/ci/pipeline/chain/populate.rb index 0405292a25b..65029f5ce7f 100644 --- a/lib/gitlab/ci/pipeline/chain/populate.rb +++ b/lib/gitlab/ci/pipeline/chain/populate.rb @@ -22,12 +22,17 @@ module Gitlab # @command.seeds_block&.call(pipeline) + ## + # Gather all runtime build/stage errors + # + if seeds_errors = pipeline.stage_seeds.flat_map(&:errors).compact.presence + return error(seeds_errors.join("\n")) + end + ## # Populate pipeline with all stages, and stages with builds. # - pipeline.stage_seeds.each do |stage| - pipeline.stages << stage.to_resource - end + pipeline.stages = pipeline.stage_seeds.map(&:to_resource) if pipeline.stages.none? return error('No stages / jobs for this pipeline.') diff --git a/lib/gitlab/ci/pipeline/seed/base.rb b/lib/gitlab/ci/pipeline/seed/base.rb index 1fd3a61017f..e9e22569ae0 100644 --- a/lib/gitlab/ci/pipeline/seed/base.rb +++ b/lib/gitlab/ci/pipeline/seed/base.rb @@ -13,6 +13,10 @@ module Gitlab raise NotImplementedError end + def errors + raise NotImplementedError + end + def to_resource raise NotImplementedError end diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index ab0d4c38ab6..32086735556 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -13,6 +13,7 @@ module Gitlab @pipeline = pipeline @attributes = attributes @previous_stages = previous_stages + @needs_attributes = dig(:needs_attributes) @only = Gitlab::Ci::Build::Policy .fabricate(attributes.delete(:only)) @@ -27,8 +28,15 @@ module Gitlab def included? strong_memoize(:inclusion) do all_of_only? && - none_of_except? && - all_of_needs? + none_of_except? + end + end + + def errors + return unless included? + + strong_memoize(:errors) do + needs_errors end end @@ -48,25 +56,6 @@ module Gitlab @attributes.to_h.dig(:options, :trigger).present? end - def all_of_only? - @only.all? { |spec| spec.satisfied_by?(@pipeline, self) } - end - - def none_of_except? - @except.none? { |spec| spec.satisfied_by?(@pipeline, self) } - end - - def all_of_needs? - return true unless Feature.enabled?(:ci_dag_support, @pipeline.project) - return true if dig(:needs_attributes).nil? - - dig(:needs_attributes).all? do |need| - @previous_stages.any? do |stage| - stage.seeds_names.include?(need[:name]) - end - end - end - def to_resource strong_memoize(:resource) do if bridge? @@ -76,6 +65,29 @@ module Gitlab end end end + + private + + def all_of_only? + @only.all? { |spec| spec.satisfied_by?(@pipeline, self) } + end + + def none_of_except? + @except.none? { |spec| spec.satisfied_by?(@pipeline, self) } + end + + def needs_errors + return unless Feature.enabled?(:ci_dag_support, @pipeline.project) + return if @needs_attributes.nil? + + @needs_attributes.flat_map do |need| + result = @previous_stages.any? do |stage| + stage.seeds_names.include?(need[:name]) + end + + "#{name}: needs '#{need[:name]}'" unless result + end.compact + end end end end diff --git a/lib/gitlab/ci/pipeline/seed/stage.rb b/lib/gitlab/ci/pipeline/seed/stage.rb index 7c737027445..b600df2f656 100644 --- a/lib/gitlab/ci/pipeline/seed/stage.rb +++ b/lib/gitlab/ci/pipeline/seed/stage.rb @@ -33,6 +33,12 @@ module Gitlab end end + def errors + strong_memoize(:errors) do + seeds.flat_map(&:errors).compact + end + end + def seeds_names strong_memoize(:seeds_names) do seeds.map(&:name).to_set diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 762025f9bd9..e0fbd2e7369 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -396,7 +396,14 @@ describe Gitlab::Ci::Pipeline::Seed::Build do end context 'when build job is not present in prior stages' do - it { is_expected.not_to be_included } + it "is included" do + is_expected.to be_included + end + + it "returns an error" do + expect(subject.errors).to contain_exactly( + "rspec: needs 'build'") + end end context 'when build job is part of prior stages' do @@ -414,7 +421,13 @@ describe Gitlab::Ci::Pipeline::Seed::Build do let(:previous_stages) { [stage_seed] } - it { is_expected.to be_included } + it "is included" do + is_expected.to be_included + end + + it "does not have errors" do + expect(subject.errors).to be_empty + end end end end diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb index 6fba9f37d91..a13335f63d5 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb @@ -121,6 +121,16 @@ describe Gitlab::Ci::Pipeline::Seed::Stage do end end + describe '#seeds_errors' do + it 'returns all errors from seeds' do + expect(subject.seeds.first) + .to receive(:errors) { ["build error"] } + + expect(subject.errors).to contain_exactly( + "build error") + end + end + describe '#to_resource' do it 'builds a valid stage object with all builds' do subject.to_resource.save! diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 7e2f311a065..deb68899309 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -1113,7 +1113,7 @@ describe Ci::CreatePipelineService do test_a: { stage: "test", script: "ls", - only: %w[master feature tags], + only: %w[master feature], needs: %w[build_a] }, deploy: { @@ -1143,6 +1143,7 @@ describe Ci::CreatePipelineService do it 'does not create a pipeline as test_a depends on build_a' do expect(pipeline).not_to be_persisted expect(pipeline.builds).to be_empty + expect(pipeline.errors[:base]).to contain_exactly("test_a: needs 'build_a'") end end