Merge branch 'show-commit-status-from-latest-pipeline' into 'master'

Show commit status from latest pipeline

Show commit status from latest pipeline rather than compound status from all pipelines.

Closes #20560

See merge request !7333

8 files changed, 105 insertions, 33 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 54f73171fd4..48354cdbefb 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -88,8 +88,24 @@ module Ci
     end
 
     # ref can't be HEAD or SHA, can only be branch/tag name
+    scope :latest, ->(ref = nil) do
+      max_id = unscope(:select)
+        .select("max(#{quoted_table_name}.id)")
+        .group(:ref, :sha)
+
+      if ref
+        where(id: max_id, ref: ref)
+      else
+        where(id: max_id)
+      end
+    end
+
+    def self.latest_status(ref = nil)
+      latest(ref).status
+    end
+
     def self.latest_successful_for(ref)
-      where(ref: ref).order(id: :desc).success.first
+      success.latest(ref).first
     end
 
     def self.truncate_sha(sha)
diff --git a/app/models/commit.rb b/app/models/commit.rb
index 1831cc7e175..69cfc47f5bf 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -228,13 +228,9 @@ class Commit
   def status(ref = nil)
     @statuses ||= {}
 
-    if @statuses.key?(ref)
-      @statuses[ref]
-    elsif ref
-      @statuses[ref] = pipelines.where(ref: ref).status
-    else
-      @statuses[ref] = pipelines.status
-    end
+    return @statuses[ref] if @statuses.key?(ref)
+
+    @statuses[ref] = pipelines.latest_status(ref)
   end
 
   def revert_branch_name
@@ -270,7 +266,7 @@ class Commit
     @merged_merge_request_hash ||= Hash.new do |hash, user|
       hash[user] = merged_merge_request_no_cache(user)
     end
-    
+
     @merged_merge_request_hash[current_user]
   end
 
diff --git a/app/services/ci/image_for_build_service.rb b/app/services/ci/image_for_build_service.rb
index 75d847d5bee..240ddabec36 100644
--- a/app/services/ci/image_for_build_service.rb
+++ b/app/services/ci/image_for_build_service.rb
@@ -1,13 +1,13 @@
 module Ci
   class ImageForBuildService
     def execute(project, opts)
-      sha = opts[:sha] || ref_sha(project, opts[:ref])
-
+      ref = opts[:ref]
+      sha = opts[:sha] || ref_sha(project, ref)
       pipelines = project.pipelines.where(sha: sha)
-      pipelines = pipelines.where(ref: opts[:ref]) if opts[:ref]
-      image_name = image_for_status(pipelines.status)
 
+      image_name = image_for_status(pipelines.latest_status(ref))
       image_path = Rails.root.join('public/ci', image_name)
+
       OpenStruct.new(path: image_path, name: image_name)
     end
 
diff --git a/changelogs/unreleased/show-commit-status-from-latest-pipeline.yml b/changelogs/unreleased/show-commit-status-from-latest-pipeline.yml
new file mode 100644
index 00000000000..bbd7a217493
--- /dev/null
+++ b/changelogs/unreleased/show-commit-status-from-latest-pipeline.yml
@@ -0,0 +1,4 @@
+---
+title: Show commit status from latest pipeline
+merge_request: 7333
+author:
diff --git a/lib/gitlab/badge/build/status.rb b/lib/gitlab/badge/build/status.rb
index 50aa45e5406..b762d85b6e5 100644
--- a/lib/gitlab/badge/build/status.rb
+++ b/lib/gitlab/badge/build/status.rb
@@ -20,8 +20,8 @@ module Gitlab
 
         def status
           @project.pipelines
-            .where(sha: @sha, ref: @ref)
-            .status || 'unknown'
+            .where(sha: @sha)
+            .latest_status(@ref) || 'unknown'
         end
 
         def metadata
diff --git a/spec/lib/gitlab/badge/build/status_spec.rb b/spec/lib/gitlab/badge/build/status_spec.rb
index 38eebb2a176..70f03021d36 100644
--- a/spec/lib/gitlab/badge/build/status_spec.rb
+++ b/spec/lib/gitlab/badge/build/status_spec.rb
@@ -69,8 +69,8 @@ describe Gitlab::Badge::Build::Status do
         new_build.success!
       end
 
-      it 'reports the compound status' do
-        expect(badge.status).to eq 'failed'
+      it 'does not take outdated pipeline into account' do
+        expect(badge.status).to eq 'success'
       end
     end
   end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index e78ae14b737..52dd41065e9 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -381,6 +381,65 @@ describe Ci::Pipeline, models: true do
     end
   end
 
+  shared_context 'with some outdated pipelines' do
+    before do
+      create_pipeline(:canceled, 'ref', 'A')
+      create_pipeline(:success, 'ref', 'A')
+      create_pipeline(:failed, 'ref', 'B')
+      create_pipeline(:skipped, 'feature', 'C')
+    end
+
+    def create_pipeline(status, ref, sha)
+      create(:ci_empty_pipeline, status: status, ref: ref, sha: sha)
+    end
+  end
+
+  describe '.latest' do
+    include_context 'with some outdated pipelines'
+
+    context 'when no ref is specified' do
+      let(:pipelines) { described_class.latest.all }
+
+      it 'returns the latest pipeline for the same ref and different sha' do
+        expect(pipelines.map(&:sha)).to contain_exactly('A', 'B', 'C')
+        expect(pipelines.map(&:status)).
+          to contain_exactly('success', 'failed', 'skipped')
+      end
+    end
+
+    context 'when ref is specified' do
+      let(:pipelines) { described_class.latest('ref').all }
+
+      it 'returns the latest pipeline for ref and different sha' do
+        expect(pipelines.map(&:sha)).to contain_exactly('A', 'B')
+        expect(pipelines.map(&:status)).
+          to contain_exactly('success', 'failed')
+      end
+    end
+  end
+
+  describe '.latest_status' do
+    include_context 'with some outdated pipelines'
+
+    context 'when no ref is specified' do
+      let(:latest_status) { described_class.latest_status }
+
+      it 'returns the latest status for the same ref and different sha' do
+        expect(latest_status).to eq(described_class.latest.status)
+        expect(latest_status).to eq('failed')
+      end
+    end
+
+    context 'when ref is specified' do
+      let(:latest_status) { described_class.latest_status('ref') }
+
+      it 'returns the latest status for ref and different sha' do
+        expect(latest_status).to eq(described_class.latest_status('ref'))
+        expect(latest_status).to eq('failed')
+      end
+    end
+  end
+
   describe '#status' do
     let!(:build) { create(:ci_build, :created, pipeline: pipeline, name: 'test') }
 
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 0935fc0561c..74b50d2908d 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -213,23 +213,19 @@ eos
   end
 
   describe '#status' do
-    context 'without arguments for compound status' do
-      shared_examples 'giving the status from pipeline' do
-        it do
-          expect(commit.status).to eq(Ci::Pipeline.status)
-        end
-      end
-
-      context 'with pipelines' do
-        let!(:pipeline) do
-          create(:ci_empty_pipeline, project: project, sha: commit.sha)
+    context 'without ref argument' do
+      before do
+        %w[success failed created pending].each do |status|
+          create(:ci_empty_pipeline,
+                 project: project,
+                 sha: commit.sha,
+                 status: status)
         end
-
-        it_behaves_like 'giving the status from pipeline'
       end
 
-      context 'without pipelines' do
-        it_behaves_like 'giving the status from pipeline'
+      it 'gives compound status from latest pipelines' do
+        expect(commit.status).to eq(Ci::Pipeline.latest_status)
+        expect(commit.status).to eq('pending')
       end
     end
 
@@ -255,8 +251,9 @@ eos
         expect(commit.status('fix')).to eq(pipeline_from_fix.status)
       end
 
-      it 'gives compound status if ref is nil' do
-        expect(commit.status(nil)).to eq(commit.status)
+      it 'gives compound status from latest pipelines if ref is nil' do
+        expect(commit.status(nil)).to eq(Ci::Pipeline.latest_status)
+        expect(commit.status(nil)).to eq('failed')
       end
     end
   end