Merge branch 'artifact-download-from-pipeline-id-with-needs' into 'master'

Allow artifacts download from specific pipeline ID

See merge request gitlab-org/gitlab!47210

app/models/ci/build_dependencies.rb

@@ -9,7 +9,7 @@ module Ci
     end
 
     def all
-      (local + cross_pipeline).uniq
+      (local + cross_project).uniq
     end
 
     # Dependencies local to the given pipeline
@@ -23,8 +23,8 @@ module Ci
       deps
     end
 
-    # Dependencies that are defined in other pipelines
-    def cross_pipeline
+    # Dependencies that are defined by project and ref
+    def cross_project
       []
     end
 
@@ -33,7 +33,7 @@ module Ci
     end
 
     def valid?
-      valid_local? && valid_cross_pipeline?
+      valid_local? && valid_cross_project?
     end
 
     private
@@ -50,7 +50,7 @@ module Ci
       local.all?(&:valid_dependency?)
     end
 
-    def valid_cross_pipeline?
+    def valid_cross_project?
       true
     end
 

ee/app/models/ee/ci/build_dependencies.rb

@@ -7,23 +7,23 @@ module EE
       extend ::Gitlab::Utils::Override
       include ::Gitlab::Utils::StrongMemoize
 
-      LIMIT = ::Gitlab::Ci::Config::Entry::Needs::NEEDS_CROSS_DEPENDENCIES_LIMIT
+      LIMIT = ::Gitlab::Ci::Config::Entry::Needs::NEEDS_CROSS_PROJECT_DEPENDENCIES_LIMIT
 
-      override :cross_pipeline
-      def cross_pipeline
-        strong_memoize(:cross_pipeline) do
-          fetch_cross_pipeline
+      override :cross_project
+      def cross_project
+        strong_memoize(:cross_project) do
+          fetch_cross_project
         end
       end
 
       private
 
-      override :valid_cross_pipeline?
-      def valid_cross_pipeline?
-        cross_pipeline.size == specified_cross_pipeline_dependencies.size
+      override :valid_cross_project?
+      def valid_cross_project?
+        cross_project.size == specified_cross_project_dependencies.size
       end
 
-      def fetch_cross_pipeline
+      def fetch_cross_project
         return [] unless processable.user_id
         return [] unless project.feature_available?(:cross_project_pipelines)
 
@@ -33,7 +33,7 @@ module EE
       end
 
       def cross_dependencies_relationship
-        deps = specified_cross_pipeline_dependencies
+        deps = specified_cross_project_dependencies
         return model_class.none unless deps.any?
 
         relationship_fragments = build_cross_dependencies_fragments(deps, model_class.latest.success)
@@ -66,7 +66,11 @@ module EE
         -> { processable.simple_variables_without_dependencies }
       end
 
-      def specified_cross_pipeline_dependencies
+      def specified_cross_project_dependencies
+        specified_cross_dependencies.select { |dep| dep[:project] }
+      end
+
+      def specified_cross_dependencies
         Array(processable.options[:cross_dependencies])
       end
     end

ee/lib/ee/gitlab/ci/config/entry/need.rb

@@ -13,12 +13,22 @@ module EE
               # needs:pipeline: other/project
               strategy :BridgeHash,
                 class: EE::Gitlab::Ci::Config::Entry::Need::BridgeHash,
-                if: -> (config) { config.is_a?(Hash) && !config.key?(:job) && !config.key?(:project) }
+                if: -> (config) { config.is_a?(Hash) && bridge_to_upstream_pipeline?(config) }
 
               # When defining DAG dependency across project/ref
-              strategy :CrossDependency,
-                class: EE::Gitlab::Ci::Config::Entry::Need::CrossDependency,
-                if: -> (config) { config.is_a?(Hash) && (config.key?(:project) || config.key?(:ref)) }
+              strategy :CrossProjectDependency,
+                class: EE::Gitlab::Ci::Config::Entry::Need::CrossProjectDependency,
+                if: -> (config) { config.is_a?(Hash) && cross_project_need?(config) }
+            end
+
+            class_methods do
+              def bridge_to_upstream_pipeline?(config)
+                !config.key?(:job) && !config.key?(:project)
+              end
+
+              def cross_project_need?(config)
+                config.key?(:project) || config.key?(:ref)
+              end
             end
 
             class BridgeHash < ::Gitlab::Config::Entry::Node
@@ -39,7 +49,7 @@ module EE
               end
             end
 
-            class CrossDependency < ::Gitlab::Config::Entry::Node
+            class CrossProjectDependency < ::Gitlab::Config::Entry::Node
               include ::Gitlab::Config::Entry::Validatable
               include ::Gitlab::Config::Entry::Attributable
 

ee/lib/ee/gitlab/ci/config/entry/needs.rb

@@ -8,14 +8,16 @@ module EE
           module Needs
             extend ActiveSupport::Concern
 
-            NEEDS_CROSS_DEPENDENCIES_LIMIT = 5
+            NEEDS_CROSS_PROJECT_DEPENDENCIES_LIMIT = 5
 
             prepended do
               validations do
                 validate on: :composed do
                   cross_dependencies = value[:cross_dependency].to_a
-                  if cross_dependencies.size > NEEDS_CROSS_DEPENDENCIES_LIMIT
-                    errors.add(:config, "must be less than or equal to #{NEEDS_CROSS_DEPENDENCIES_LIMIT}")
+                  cross_project_dependencies = cross_dependencies.select { |dep| dep[:project] }
+
+                  if cross_project_dependencies.size > NEEDS_CROSS_PROJECT_DEPENDENCIES_LIMIT
+                    errors.add(:config, "must be less than or equal to #{NEEDS_CROSS_PROJECT_DEPENDENCIES_LIMIT}")
                   end
                 end
               end

ee/spec/lib/ee/gitlab/ci/config/entry/need_spec.rb

@@ -36,7 +36,7 @@ RSpec.describe ::Gitlab::Ci::Config::Entry::Need do
     end
   end
 
-  context 'with CrossDependency config' do
+  context 'with CrossProjectDependency config' do
     describe '#artifacts' do
       using RSpec::Parameterized::TableSyntax
 
@@ -117,7 +117,7 @@ RSpec.describe ::Gitlab::Ci::Config::Entry::Need do
           describe '#errors' do
             subject(:errors) { need.errors }
 
-            let(:error_message) { "cross dependency #{attribute} #{error}" }
+            let(:error_message) { "cross project dependency #{attribute} #{error}" }
 
             it { is_expected.to(be_empty)               if validity }
             it { is_expected.to(include(error_message)) unless validity }

ee/spec/lib/ee/gitlab/ci/config/entry/needs_spec.rb

@@ -45,28 +45,74 @@ RSpec.describe ::Gitlab::Ci::Config::Entry::Needs do
       end
     end
 
-    context 'with too many cross dependencies' do
-      let(:limit) { described_class::NEEDS_CROSS_DEPENDENCIES_LIMIT }
+    context 'cross dependencies limit' do
+      context 'when enforcing limit for cross project dependencies' do
+        let(:limit) { described_class::NEEDS_CROSS_PROJECT_DEPENDENCIES_LIMIT }
 
-      let(:config) do
-        Array.new(limit.next) do |index|
-          {
-            project: "project-#{index}",
-            job: 'job-1',
-            ref: 'master',
-            artifacts: true
-          }
+        context 'when limit is exceeded' do
+          let(:config) do
+            Array.new(limit.next) do |index|
+              {
+                project: "project-#{index}",
+                job: 'job-1',
+                ref: 'master',
+                artifacts: true
+              }
+            end
+          end
+
+          describe '#valid?' do
+            it { is_expected.not_to be_valid }
+          end
+
+          describe '#errors' do
+            it 'returns error about incorrect type' do
+              expect(needs.errors).to contain_exactly(
+                "needs config must be less than or equal to #{limit}")
+            end
+          end
         end
-      end
 
-      describe '#valid?' do
-        it { is_expected.not_to be_valid }
+        context 'when limit is not exceeded' do
+          let(:config) do
+            Array.new(limit) do |index|
+              {
+                project: "project-#{index}",
+                job: 'job-1',
+                ref: 'master',
+                artifacts: true
+              }
+            end + [
+              { pipeline: '$UPSTREAM_PIPELINE_ID', job: 'rspec' }
+            ]
+          end
+
+          it 'does not count cross pipeline dependencies' do
+            expect(subject).to be_valid
+          end
+        end
       end
 
-      describe '#errors' do
-        it 'returns error about incorrect type' do
-          expect(needs.errors).to contain_exactly(
-            "needs config must be less than or equal to #{limit}")
+      context 'when enforcing limit for cross pipeline dependencies' do
+        let(:limit) { described_class::NEEDS_CROSS_PIPELINE_DEPENDENCIES_LIMIT }
+
+        context 'when limit is not exceeded' do
+          let(:config) do
+            Array.new(limit) do |index|
+              { pipeline: "$UPSTREAM_PIPELINE_#{index}", job: 'job-1' }
+            end + [
+              {
+                project: 'org/the-project',
+                job: 'build',
+                ref: 'master',
+                artifacts: true
+              }
+            ]
+          end
+
+          it 'does not count cross project dependencies' do
+            expect(subject).to be_valid
+          end
         end
       end
     end

ee/spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -201,6 +201,54 @@ RSpec.describe Gitlab::Ci::YamlProcessor do
           'jobs:test:needs:need ref should be a string')
       end
     end
+
+    describe 'cross pipeline needs' do
+      context 'when job is not present' do
+        let(:config) do
+          {
+            rspec: {
+              stage: 'test',
+              script: 'rspec',
+              needs: [
+                { pipeline: '$UPSTREAM_PIPELINE_ID' }
+              ]
+            }
+          }
+        end
+
+        it 'returns an error' do
+          expect(subject).not_to be_valid
+          # This currently shows a confusing error message because a conflict of syntax
+          # with upstream pipeline status mirroring: https://gitlab.com/gitlab-org/gitlab/-/issues/280853
+          expect(subject.errors).to include(/:needs config uses invalid types: bridge/)
+        end
+      end
+    end
+
+    describe 'with cross project and cross pipeline needs' do
+      let(:config) do
+        {
+          rspec: {
+            stage: 'test',
+            script: 'rspec',
+            needs: [
+              { pipeline: '$UPSTREAM_PIPELINE_ID', job: 'test' },
+              { project: 'org/the-project', ref: 'master', job: 'build', artifacts: true }
+            ]
+          }
+        }
+      end
+
+      it 'returns a valid specification' do
+        expect(subject).to be_valid
+
+        rspec = subject.builds.last
+        expect(rspec.dig(:options, :cross_dependencies)).to eq([
+          { pipeline: '$UPSTREAM_PIPELINE_ID', job: 'test', artifacts: true },
+          { project: 'org/the-project', ref: 'master', job: 'build', artifacts: true }
+        ])
+      end
+    end
   end
 
   describe 'Secrets' do

ee/spec/models/ee/ci/build_dependencies_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Ci::BuildDependencies do
-  describe '#cross_pipeline' do
+  describe '#cross_project' do
     let_it_be(:user) { create(:user) }
     let_it_be(:project, refind: true) { create(:project, :repository) }
     let(:dependencies) { }
@@ -26,7 +26,7 @@ RSpec.describe Ci::BuildDependencies do
         options: { cross_dependencies: dependencies })
     end
 
-    subject { described_class.new(job).cross_pipeline }
+    subject { described_class.new(job).cross_project }
 
     before do
       project.add_developer(user)
@@ -53,7 +53,7 @@ RSpec.describe Ci::BuildDependencies do
       it { is_expected.to be_empty }
     end
 
-    context 'with cross_dependencies to the same pipeline' do
+    context 'with cross_dependencies to the same project' do
       let!(:dependency) do
         create(:ci_build, :success,
           pipeline: pipeline,
@@ -108,7 +108,7 @@ RSpec.describe Ci::BuildDependencies do
       end
     end
 
-    context 'with cross_dependencies to another pipeline in same project' do
+    context 'with cross_dependencies to another ref in same project' do
       let(:another_pipeline) do
         create(:ci_pipeline,
           project: project,
@@ -222,9 +222,71 @@ RSpec.describe Ci::BuildDependencies do
       end
     end
 
+    context 'with both cross project and cross pipeline dependencies' do
+      let(:other_project) { create(:project, :repository) }
+
+      let(:other_project_pipeline) do
+        create(:ci_pipeline,
+          project: other_project,
+          sha: other_project.commit.id,
+          ref: other_project.default_branch,
+          status: 'success',
+          user: user)
+      end
+
+      let!(:cross_project_dependency) do
+        create(:ci_build, :success,
+          pipeline: other_project_pipeline,
+          ref: other_project_pipeline.ref,
+          name: 'deploy',
+          stage_idx: 4,
+          stage: 'deploy',
+          user: user)
+      end
+
+      let(:upstream_pipeline) do
+        create(:ci_pipeline,
+          project: project,
+          sha: project.commit.id,
+          ref: project.default_branch,
+          status: 'success',
+          user: user)
+      end
+
+      let!(:upstream_pipeline_dependency) do
+        create(:ci_build, :success,
+          pipeline: upstream_pipeline,
+          ref: upstream_pipeline.ref,
+          name: 'build',
+          stage_idx: 1,
+          stage: 'build',
+          user: user)
+      end
+
+      let(:dependencies) do
+        [
+          { pipeline: '$UPSTREAM_PIPELINE_ID', job: 'build', artifacts: true },
+          { project: other_project.full_path, ref: other_project.default_branch, job: 'deploy', artifacts: true }
+        ]
+      end
+
+      before do
+        job.yaml_variables.push(key: 'UPSTREAM_PIPELINE_ID', value: upstream_pipeline.id.to_s, public: true)
+        job.save!
+
+        other_project.add_developer(user)
+      end
+
+      # TODO: In a follow-up MR we are adding support to querying pipelines in the same
+      # project.
+      it 'temporarily ignores cross pipeline dependencies' do
+        is_expected.to contain_exactly(cross_project_dependency)
+      end
+    end
+
     context 'with too many cross_dependencies' do
       let(:cross_dependencies_limit) do
-        ::Gitlab::Ci::Config::Entry::Needs::NEEDS_CROSS_DEPENDENCIES_LIMIT
+        ::Gitlab::Ci::Config::Entry::Needs::NEEDS_CROSS_PROJECT_DEPENDENCIES_LIMIT
       end
 
       before do

lib/gitlab/ci/config/entry/need.rb

@@ -8,7 +8,19 @@ module Gitlab
           strategy :JobString, if: -> (config) { config.is_a?(String) }
 
           strategy :JobHash,
-            if: -> (config) { config.is_a?(Hash) && config.key?(:job) && !(config.key?(:project) || config.key?(:ref)) }
+            if: -> (config) { config.is_a?(Hash) && same_pipeline_need?(config) }
+
+          strategy :CrossPipelineDependency,
+            if: -> (config) { config.is_a?(Hash) && cross_pipeline_need?(config) }
+
+          def self.same_pipeline_need?(config)
+            config.key?(:job) &&
+              !(config.key?(:project) || config.key?(:ref) || config.key?(:pipeline))
+          end
+
+          def self.cross_pipeline_need?(config)
+            config.key?(:job) && config.key?(:pipeline) && !config.key?(:project)
+          end
 
           class JobString < ::Gitlab::Config::Entry::Node
             include ::Gitlab::Config::Entry::Validatable
@@ -50,6 +62,30 @@ module Gitlab
             end
           end
 
+          class CrossPipelineDependency < ::Gitlab::Config::Entry::Node
+            include ::Gitlab::Config::Entry::Validatable
+            include ::Gitlab::Config::Entry::Attributable
+
+            ALLOWED_KEYS = %i[pipeline job artifacts].freeze
+            attributes :pipeline, :job, :artifacts
+
+            validations do
+              validates :config, presence: true
+              validates :config, allowed_keys: ALLOWED_KEYS
+              validates :pipeline, type: String, presence: true
+              validates :job, type: String, presence: true
+              validates :artifacts, boolean: true, allow_nil: true
+            end
+
+            def type
+              :cross_dependency
+            end
+
+            def value
+              super.merge(artifacts: artifacts || artifacts.nil?)
+            end
+          end
+
           class UnknownStrategy < ::Gitlab::Config::Entry::Node
             def type
             end

lib/gitlab/ci/config/entry/needs.rb

@@ -10,6 +10,8 @@ module Gitlab
         class Needs < ::Gitlab::Config::Entry::ComposableArray
           include ::Gitlab::Config::Entry::Validatable
 
+          NEEDS_CROSS_PIPELINE_DEPENDENCIES_LIMIT = 5
+
           validations do
             validate do
               unless config.is_a?(Hash) || config.is_a?(Array)
@@ -27,6 +29,15 @@ module Gitlab
                 errors.add(:config, "uses invalid types: #{extra_keys.join(', ')}")
               end
             end
+
+            validate on: :composed do
+              cross_dependencies = value[:cross_dependency].to_a
+              cross_pipeline_dependencies = cross_dependencies.select { |dep| dep[:pipeline] }
+
+              if cross_pipeline_dependencies.size > NEEDS_CROSS_PIPELINE_DEPENDENCIES_LIMIT
+                errors.add(:config, "must be less than or equal to #{NEEDS_CROSS_PIPELINE_DEPENDENCIES_LIMIT}")
+              end
+            end
           end
 
           def value

spec/lib/gitlab/ci/config/entry/need_spec.rb

@@ -165,6 +165,45 @@ RSpec.describe ::Gitlab::Ci::Config::Entry::Need do
     end
   end
 
+  context 'with cross pipeline artifacts needs' do
+    context 'when pipeline is provided' do
+      context 'when job is provided' do
+        let(:config) { { job: 'job_name', pipeline: '$THE_PIPELINE_ID' } }
+
+        it { is_expected.to be_valid }
+
+        it 'sets artifacts:true by default' do
+          expect(need.value).to eq(job: 'job_name', pipeline: '$THE_PIPELINE_ID', artifacts: true)
+        end
+
+        it 'sets the type as cross_dependency' do
+          expect(need.type).to eq(:cross_dependency)
+        end
+      end
+
+      context 'when artifacts is provided' do
+        let(:config) { { job: 'job_name', pipeline: '$THE_PIPELINE_ID', artifacts: false } }
+
+        it { is_expected.to be_valid }
+
+        it 'returns the correct value' do
+          expect(need.value).to eq(job: 'job_name', pipeline: '$THE_PIPELINE_ID', artifacts: false)
+        end
+      end
+    end
+
+    context 'when config contains not allowed keys' do
+      let(:config) { { job: 'job_name', pipeline: '$THE_PIPELINE_ID', something: 'else' } }
+
+      it { is_expected.not_to be_valid }
+
+      it 'returns an error' do
+        expect(need.errors)
+          .to contain_exactly('cross pipeline dependency config contains unknown keys: something')
+      end
+    end
+  end
+
   context 'when need config is not a string or a hash' do
     let(:config) { :job_name }
 

spec/lib/gitlab/ci/config/entry/needs_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe ::Gitlab::Ci::Config::Entry::Needs do
   subject(:needs) { described_class.new(config) }
 
   before do
-    needs.metadata[:allowed_needs] = %i[job]
+    needs.metadata[:allowed_needs] = %i[job cross_dependency]
   end
 
   describe 'validations' do
@@ -66,6 +66,27 @@ RSpec.describe ::Gitlab::Ci::Config::Entry::Needs do
         end
       end
     end
+
+    context 'with too many cross pipeline dependencies' do
+      let(:limit) { described_class::NEEDS_CROSS_PIPELINE_DEPENDENCIES_LIMIT }
+
+      let(:config) do
+        Array.new(limit.next) do |index|
+          { pipeline: "$UPSTREAM_PIPELINE_#{index}", job: 'job-1' }
+        end
+      end
+
+      describe '#valid?' do
+        it { is_expected.not_to be_valid }
+      end
+
+      describe '#errors' do
+        it 'returns error about incorrect type' do
+          expect(needs.errors).to contain_exactly(
+            "needs config must be less than or equal to #{limit}")
+        end
+      end
+    end
   end
 
   describe '.compose!' do

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -2111,6 +2111,71 @@ module Gitlab
         end
       end
 
+      describe 'cross pipeline needs' do
+        context 'when configuration is valid' do
+          let(:config) do
+            <<~YAML
+            rspec:
+              stage: test
+              script: rspec
+              needs:
+                - pipeline: $THE_PIPELINE_ID
+                  job: dependency-job
+            YAML
+          end
+
+          it 'returns a valid configuration and sets artifacts: true by default' do
+            expect(subject).to be_valid
+
+            rspec = subject.build_attributes(:rspec)
+            expect(rspec.dig(:options, :cross_dependencies)).to eq(
+              [{ pipeline: '$THE_PIPELINE_ID', job: 'dependency-job', artifacts: true }]
+            )
+          end
+
+          context 'when pipeline ID is hard-coded' do
+            let(:config) do
+              <<~YAML
+              rspec:
+                stage: test
+                script: rspec
+                needs:
+                  - pipeline: "123"
+                    job: dependency-job
+              YAML
+            end
+
+            it 'returns a valid configuration and sets artifacts: true by default' do
+              expect(subject).to be_valid
+
+              rspec = subject.build_attributes(:rspec)
+              expect(rspec.dig(:options, :cross_dependencies)).to eq(
+                [{ pipeline: '123', job: 'dependency-job', artifacts: true }]
+              )
+            end
+          end
+        end
+
+        context 'when configuration is not valid' do
+          let(:config) do
+            <<~YAML
+            rspec:
+              stage: test
+              script: rspec
+              needs:
+                - pipeline: $THE_PIPELINE_ID
+                  job: dependency-job
+                  something: else
+            YAML
+          end
+
+          it 'returns an error' do
+            expect(subject).not_to be_valid
+            expect(subject.errors).to include(/:need config contains unknown keys: something/)
+          end
+        end
+      end
+
       describe "Hidden jobs" do
         let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config).execute }
 

spec/models/ci/build_dependencies_spec.rb

@@ -155,9 +155,9 @@ RSpec.describe Ci::BuildDependencies do
 
     subject { dependencies.all }
 
-    it 'returns the union of all local dependencies and any cross pipeline dependencies' do
+    it 'returns the union of all local dependencies and any cross project dependencies' do
       expect(dependencies).to receive(:local).and_return([1, 2, 3])
-      expect(dependencies).to receive(:cross_pipeline).and_return([3, 4])
+      expect(dependencies).to receive(:cross_project).and_return([3, 4])
 
       expect(subject).to contain_exactly(1, 2, 3, 4)
     end