Merge branch 'fix/status-of-pipeline-without-builds' into 'master'

Improve pipeline status in case that pipeline has no jobs

## What does this MR do?

This MR resolves problem with pipeline status when there are no build in pipeline.

This can happen when builds were skipped - for example - by using `only`/`except` keyword in `.gitlab-ci.yml`.

## What are the relevant issue numbers?

Closes #17977

See merge request !4403

CHANGELOG

@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.9.0 (unreleased)
+  - Fix pipeline status when there are no builds in pipeline
   - Fix Error 500 when using closes_issues API with an external issue tracker
   - Add more information into RSS feed for issues (Alexander Matyushentsev)
   - Bulk assign/unassign labels to issues.

app/models/ci/pipeline.rb

@@ -94,10 +94,13 @@ module Ci
     end
 
     def create_builds(user, trigger_request = nil)
+      ##
+      # We persist pipeline only if there are builds available
+      #
       return unless config_processor
-      config_processor.stages.any? do |stage|
+
-        CreateBuildsService.new(self).execute(stage, user, 'success', trigger_request).present?
+      build_builds_for_stages(config_processor.stages, user,
-      end
+                              'success', trigger_request) && save
     end
 
     def create_next_builds(build)
@@ -115,10 +118,10 @@ module Ci
       prior_builds = latest_builds.where.not(stage: next_stages)
       prior_status = prior_builds.status
 
-      # create builds for next stages based
+      # build builds for next stage that has builds available
-      next_stages.any? do |stage|
+      # and save pipeline if we have builds
-        CreateBuildsService.new(self).execute(stage, build.user, prior_status, build.trigger_request).present?
+      build_builds_for_stages(next_stages, build.user, prior_status,
-      end
+                              build.trigger_request) && save
     end
 
     def retried
@@ -139,10 +142,10 @@ module Ci
       @config_processor ||= begin
         Ci::GitlabCiYamlProcessor.new(ci_yaml_file, project.path_with_namespace)
       rescue Ci::GitlabCiYamlProcessor::ValidationError, Psych::SyntaxError => e
-        save_yaml_error(e.message)
+        self.yaml_errors = e.message
         nil
       rescue
-        save_yaml_error("Undefined error")
+        self.yaml_errors = 'Undefined error'
         nil
       end
     end
@@ -169,6 +172,17 @@ module Ci
 
     private
 
+    def build_builds_for_stages(stages, user, status, trigger_request)
+      ##
+      # Note that `Array#any?` implements a short circuit evaluation, so we
+      # build builds only for the first stage that has builds available.
+      #
+      stages.any? do |stage|
+        CreateBuildsService.new(self)
+          .execute(stage, user, status, trigger_request).present?
+      end
+    end
+
     def update_state
       statuses.reload
       self.status = if yaml_errors.blank?
@@ -181,11 +195,5 @@ module Ci
       self.duration = statuses.latest.duration
       save
     end
-
-    def save_yaml_error(error)
-      return if self.yaml_errors?
-      self.yaml_errors = error
-      update_state
-    end
   end
 end

app/services/ci/create_builds_service.rb

@@ -2,10 +2,11 @@ module Ci
   class CreateBuildsService
     def initialize(pipeline)
       @pipeline = pipeline
+      @config = pipeline.config_processor
     end
 
     def execute(stage, user, status, trigger_request = nil)
-      builds_attrs = config_processor.builds_for_stage_and_ref(stage, @pipeline.ref, @pipeline.tag, trigger_request)
+      builds_attrs = @config.builds_for_stage_and_ref(stage, @pipeline.ref, @pipeline.tag, trigger_request)
 
       # check when to create next build
       builds_attrs = builds_attrs.select do |build_attrs|
@@ -19,34 +20,37 @@ module Ci
         end
       end
 
-      builds_attrs.map do |build_attrs|
+      # don't create the same build twice
-        # don't create the same build twice
+      builds_attrs.reject! do |build_attrs|
-        unless @pipeline.builds.find_by(ref: @pipeline.ref, tag: @pipeline.tag,
+        @pipeline.builds.find_by(ref: @pipeline.ref,
-                                        trigger_request: trigger_request, name: build_attrs[:name])
+                                 tag: @pipeline.tag,
-          build_attrs.slice!(:name,
+                                 trigger_request: trigger_request,
-                             :commands,
+                                 name: build_attrs[:name])
-                             :tag_list,
-                             :options,
-                             :allow_failure,
-                             :stage,
-                             :stage_idx,
-                             :environment)
-
-          build_attrs.merge!(ref: @pipeline.ref,
-                             tag: @pipeline.tag,
-                             trigger_request: trigger_request,
-                             user: user,
-                             project: @pipeline.project)
-
-          @pipeline.builds.create!(build_attrs)
-        end
       end
-    end
 
-    private
+      builds_attrs.map do |build_attrs|
+        build_attrs.slice!(:name,
+                           :commands,
+                           :tag_list,
+                           :options,
+                           :allow_failure,
+                           :stage,
+                           :stage_idx,
+                           :environment)
 
-    def config_processor
+        build_attrs.merge!(pipeline: @pipeline,
-      @config_processor ||= @pipeline.config_processor
+                           ref: @pipeline.ref,
+                           tag: @pipeline.tag,
+                           trigger_request: trigger_request,
+                           user: user,
+                           project: @pipeline.project)
+
+        ##
+        # We do not persist new builds here.
+        # Those will be persisted when @pipeline is saved.
+        #
+        @pipeline.builds.new(build_attrs)
+      end
     end
   end
 end

app/services/ci/create_pipeline_service.rb

@@ -8,7 +8,9 @@ module Ci
         return pipeline
       end
 
-      unless commit
+      if commit
+        pipeline.sha = commit.id
+      else
         pipeline.errors.add(:base, 'Commit not found')
         return pipeline
       end
@@ -18,22 +20,18 @@ module Ci
         return pipeline
       end
 
-      begin
+      unless pipeline.config_processor
-        Ci::Pipeline.transaction do
+        pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
-          pipeline.sha = commit.id
+        return pipeline
-
-          unless pipeline.config_processor
-            pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
-            raise ActiveRecord::Rollback
-          end
-
-          pipeline.save!
-          pipeline.create_builds(current_user)
-        end
-      rescue
-        pipeline.errors.add(:base, 'The pipeline could not be created. Please try again.')
       end
 
+      pipeline.save!
+
+      unless pipeline.create_builds(current_user)
+        pipeline.errors.add(:base, 'No builds for this pipeline.')
+      end
+
+      pipeline.save
       pipeline
     end
 

app/services/create_commit_builds_service.rb

@@ -1,15 +1,11 @@
 class CreateCommitBuildsService
   def execute(project, user, params)
-    return false unless project.builds_enabled?
+    return unless project.builds_enabled?
 
     before_sha = params[:checkout_sha] || params[:before]
     sha = params[:checkout_sha] || params[:after]
     origin_ref = params[:ref]
 
-    unless origin_ref && sha.present?
-      return false
-    end
-
     ref = Gitlab::Git.ref_name(origin_ref)
     tag = Gitlab::Git.tag_ref?(origin_ref)
 
@@ -18,23 +14,50 @@ class CreateCommitBuildsService
       return false
     end
 
-    pipeline = Ci::Pipeline.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
+    @pipeline = Ci::Pipeline.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
 
-    # Skip creating pipeline when no gitlab-ci.yml is found
+    ##
-    unless pipeline.ci_yaml_file
+    # Skip creating pipeline if no gitlab-ci.yml is found
+    #
+    unless @pipeline.ci_yaml_file
       return false
     end
 
-    # Create a new pipeline
+    ##
-    pipeline.save!
-
     # Skip creating builds for commits that have [ci skip]
-    unless pipeline.skip_ci?
+    # but save pipeline object
-      # Create builds for commit
+    #
-      pipeline.create_builds(user)
+    if @pipeline.skip_ci?
+      return save_pipeline!
     end
 
-    pipeline.touch
+    ##
-    pipeline
+    # Skip creating builds when CI config is invalid
+    # but save pipeline object
+    #
+    unless @pipeline.config_processor
+      return save_pipeline!
+    end
+
+    ##
+    # Skip creating pipeline object if there are no builds for it.
+    #
+    unless @pipeline.create_builds(user)
+      @pipeline.errors.add(:base, 'No builds created')
+      return false
+    end
+
+    save_pipeline!
+  end
+
+  private
+
+  ##
+  # Create a new pipeline and touch object to calculate status
+  #
+  def save_pipeline!
+    @pipeline.save!
+    @pipeline.touch
+    @pipeline
   end
 end

lib/ci/gitlab_ci_yaml_processor.rb

@@ -30,7 +30,10 @@ module Ci
     end
 
     def builds_for_stage_and_ref(stage, ref, tag = false, trigger_request = nil)
-      builds.select{|build| build[:stage] == stage && process?(build[:only], build[:except], ref, tag, trigger_request)}
+      builds.select do |build|
+        build[:stage] == stage &&
+          process?(build[:only], build[:except], ref, tag, trigger_request)
+      end
     end
 
     def builds

spec/models/ci/pipeline_spec.rb

@@ -258,6 +258,19 @@ describe Ci::Pipeline, models: true do
         end
       end
     end
+
+    context 'when no builds created' do
+      let(:pipeline) { build(:ci_pipeline) }
+
+      before do
+        stub_ci_pipeline_yaml_file(YAML.dump(before_script: ['ls']))
+      end
+
+      it 'returns false' do
+        expect(pipeline.create_builds(nil)).to be_falsey
+        expect(pipeline).not_to be_persisted
+      end
+    end
   end
 
   describe "#finished_at" do

spec/services/ci/create_builds_service_spec.rb

@@ -9,7 +9,7 @@ describe Ci::CreateBuildsService, services: true do
     #
 
     subject do
-      described_class.new(pipeline).execute('test', nil, user, status)
+      described_class.new(pipeline).execute('test', user, status, nil)
     end
 
     context 'next builds available' do
@@ -17,6 +17,10 @@ describe Ci::CreateBuildsService, services: true do
 
       it { is_expected.to be_an_instance_of Array }
       it { is_expected.to all(be_an_instance_of Ci::Build) }
+
+      it 'does not persist created builds' do
+        expect(subject.first).not_to be_persisted
+      end
     end
 
     context 'builds skipped' do

spec/services/create_commit_builds_service_spec.rb

@@ -39,7 +39,7 @@ describe CreateCommitBuildsService, services: true do
       end
 
       it "creates commit if there is no appropriate job but deploy job has right ref setting" do
-        config = YAML.dump({ deploy: { deploy: "ls", only: ["0_1"] } })
+        config = YAML.dump({ deploy: { script: "ls", only: ["0_1"] } })
         stub_ci_pipeline_yaml_file(config)
 
         result = service.execute(project, user,
@@ -81,7 +81,7 @@ describe CreateCommitBuildsService, services: true do
       expect(pipeline.yaml_errors).not_to be_nil
     end
 
-    describe :ci_skip? do
+    context 'when commit contains a [ci skip] directive' do
       let(:message) { "some message[ci skip]" }
 
       before do
@@ -171,5 +171,24 @@ describe CreateCommitBuildsService, services: true do
       expect(pipeline.status).to eq("failed")
       expect(pipeline.builds.any?).to be false
     end
+
+    context 'when there are no jobs for this pipeline' do
+      before do
+        config = YAML.dump({ test: { script: 'ls', only: ['feature'] } })
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      it 'does not create a new pipeline' do
+        result = service.execute(project, user,
+                                 ref: 'refs/heads/master',
+                                 before: '00000000',
+                                 after: '31das312',
+                                 commits: [{ message: 'some msg' }])
+
+        expect(result).to be_falsey
+        expect(Ci::Build.all).to be_empty
+        expect(Ci::Pipeline.count).to eq(0)
+      end
+    end
   end
 end