From b63f2794f076a7394c8a000829a632bcffef2b00 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 23 Dec 2016 16:53:56 +0800 Subject: [PATCH 1/4] 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 --- app/models/ci/pipeline.rb | 12 +++++++----- app/models/project.rb | 2 +- .../unreleased/fix-latest-pipeine-ordering.yml | 4 ++++ spec/models/ci/pipeline_spec.rb | 14 ++++++-------- 4 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/fix-latest-pipeine-ordering.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index f2f6453b3b9..5494a8da0d9 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -93,11 +93,13 @@ module Ci .select("max(#{quoted_table_name}.id)") .group(:ref, :sha) - if ref - where(id: max_id, ref: ref) - else - where(id: max_id) - end + query = if ref + where(id: max_id, ref: ref) + else + where(id: max_id) + end + + query.order(id: :desc) end def self.latest_status(ref = nil) diff --git a/app/models/project.rb b/app/models/project.rb index 26fa20f856d..72fdd4514c4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -418,7 +418,7 @@ class Project < ActiveRecord::Base repository.commit(ref) 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) latest_pipeline = pipelines.latest_successful_for(ref) diff --git a/changelogs/unreleased/fix-latest-pipeine-ordering.yml b/changelogs/unreleased/fix-latest-pipeine-ordering.yml new file mode 100644 index 00000000000..c7c9885d55a --- /dev/null +++ b/changelogs/unreleased/fix-latest-pipeine-ordering.yml @@ -0,0 +1,4 @@ +--- +title: Fix downloading latest artifact +merge_request: 8286 +author: diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index dc377d15f15..f5d206e0a3e 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -424,20 +424,18 @@ describe Ci::Pipeline, models: true do 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') + it 'returns the latest pipelines for the same ref and different sha' do + expect(pipelines.map(&:sha)).to eq(%w[C B A]) + expect(pipelines.map(&:status)).to eq(%w[skipped failed success]) 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') + it 'returns the latest pipelines for ref and different sha' do + expect(pipelines.map(&:sha)).to eq(%w[B A]) + expect(pipelines.map(&:status)).to eq(%w[failed success]) end end end From 13d009ce545271c6d0422a8df61c74028439d8bd Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 23 Dec 2016 18:17:25 +0800 Subject: [PATCH 2/4] Prefer oneline and Rubocop prefers ternary operator Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8286/diffs#note_20433402 --- app/models/ci/pipeline.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 5494a8da0d9..6894a5763ff 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -93,13 +93,8 @@ module Ci .select("max(#{quoted_table_name}.id)") .group(:ref, :sha) - query = if ref - where(id: max_id, ref: ref) - else - where(id: max_id) - end - - query.order(id: :desc) + relation = ref ? where(ref: ref) : self + relation.where(id: max_id).order(id: :desc) end def self.latest_status(ref = nil) From 1c275a7523e5d891a8d15720386b6119d0cedf41 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 23 Dec 2016 18:30:25 +0800 Subject: [PATCH 3/4] Update description stating that ordering is important Feedback from Grzegorz --- spec/models/ci/pipeline_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index f5d206e0a3e..b28da6daabf 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -424,7 +424,7 @@ describe Ci::Pipeline, models: true do context 'when no ref is specified' do let(:pipelines) { described_class.latest.all } - it 'returns the latest pipelines for the same ref and different sha' do + it 'gives the latest pipelines for the same ref and different sha in reverse chronological order' do expect(pipelines.map(&:sha)).to eq(%w[C B A]) expect(pipelines.map(&:status)).to eq(%w[skipped failed success]) end @@ -433,7 +433,7 @@ describe Ci::Pipeline, models: true do context 'when ref is specified' do let(:pipelines) { described_class.latest('ref').all } - it 'returns the latest pipelines for ref and different sha' do + it 'gives the latest pipelines for ref and different sha in reverse chronological order' do expect(pipelines.map(&:sha)).to eq(%w[B A]) expect(pipelines.map(&:status)).to eq(%w[failed success]) end From 332b85d2df466118b62b6599bd18036d691d04ed Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 23 Dec 2016 18:49:40 +0800 Subject: [PATCH 4/4] Update the description because it's more broader Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8286/diffs#note_20435082 --- changelogs/unreleased/fix-latest-pipeine-ordering.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/fix-latest-pipeine-ordering.yml b/changelogs/unreleased/fix-latest-pipeine-ordering.yml index c7c9885d55a..3dbd1ba036a 100644 --- a/changelogs/unreleased/fix-latest-pipeine-ordering.yml +++ b/changelogs/unreleased/fix-latest-pipeine-ordering.yml @@ -1,4 +1,4 @@ --- -title: Fix downloading latest artifact +title: Fix finding the latest pipeline merge_request: 8286 author: