Commit b63f2794 authored by Lin Jen-Shin's avatar Lin Jen-Shin

Ci::Pipeline.latest order by id DESC

The name latest implies that it's reverse chronological,
and we did expect it that way.

https://gitlab.com/gitlab-org/gitlab-ce/issues/25993#note_20429761


> ok, I think markglenfletchera is correct in
> https://gitlab.com/gitlab-com/support-forum/issues/1394#note_20399939
> that `Project#latest_successful_builds_for` is giving oldest pipeline
> rather than latest pipeline. This is a ~regression introduced by !7333
> where `order(id: :desc)` was removed causing this. The offending change
> was:
> https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7333/diffs#b22732e5f39e176c7c719fe485847d0fb0564275_92_108
>
> The confusion was caused by the `latest` name implication, which
> actually didn't order anything, and I think we should add `order(id:
> :desc)` to `Ci::Pipeline.latest` otherwise it's confusing that it's not
> actually ordered.


Closes #25993
parent 358a2d8b
...@@ -93,11 +93,13 @@ module Ci ...@@ -93,11 +93,13 @@ module Ci
.select("max(#{quoted_table_name}.id)") .select("max(#{quoted_table_name}.id)")
.group(:ref, :sha) .group(:ref, :sha)
if ref query = if ref
where(id: max_id, ref: ref) where(id: max_id, ref: ref)
else else
where(id: max_id) where(id: max_id)
end end
query.order(id: :desc)
end end
def self.latest_status(ref = nil) def self.latest_status(ref = nil)
......
...@@ -418,7 +418,7 @@ class Project < ActiveRecord::Base ...@@ -418,7 +418,7 @@ class Project < ActiveRecord::Base
repository.commit(ref) repository.commit(ref)
end end
# ref can't be HEAD, can only be branch/tag name or SHA # ref can't be HEAD or SHA, can only be branch/tag name
def latest_successful_builds_for(ref = default_branch) def latest_successful_builds_for(ref = default_branch)
latest_pipeline = pipelines.latest_successful_for(ref) latest_pipeline = pipelines.latest_successful_for(ref)
......
---
title: Fix downloading latest artifact
merge_request: 8286
author:
...@@ -424,20 +424,18 @@ describe Ci::Pipeline, models: true do ...@@ -424,20 +424,18 @@ describe Ci::Pipeline, models: true do
context 'when no ref is specified' do context 'when no ref is specified' do
let(:pipelines) { described_class.latest.all } let(:pipelines) { described_class.latest.all }
it 'returns the latest pipeline for the same ref and different sha' do it 'returns the latest pipelines for the same ref and different sha' do
expect(pipelines.map(&:sha)).to contain_exactly('A', 'B', 'C') expect(pipelines.map(&:sha)).to eq(%w[C B A])
expect(pipelines.map(&:status)). expect(pipelines.map(&:status)).to eq(%w[skipped failed success])
to contain_exactly('success', 'failed', 'skipped')
end end
end end
context 'when ref is specified' do context 'when ref is specified' do
let(:pipelines) { described_class.latest('ref').all } let(:pipelines) { described_class.latest('ref').all }
it 'returns the latest pipeline for ref and different sha' do it 'returns the latest pipelines for ref and different sha' do
expect(pipelines.map(&:sha)).to contain_exactly('A', 'B') expect(pipelines.map(&:sha)).to eq(%w[B A])
expect(pipelines.map(&:status)). expect(pipelines.map(&:status)).to eq(%w[failed success])
to contain_exactly('success', 'failed')
end end
end 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