From 4bca06708d09cb2a65a840c3d7d1412a98d52416 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 13 Mar 2018 22:55:38 +0900 Subject: Clarify ambiguous with_artifacts implication --- app/models/ci/build.rb | 8 ++++---- app/models/ci/pipeline.rb | 2 +- app/models/project.rb | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index b230b7f47ef..1e2b8e4793d 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -41,12 +41,12 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } - scope :with_artifacts, ->() do + scope :with_artifacts, ->(file_type) do where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', - '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) + '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').public_send(file_type)) end - scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } - scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } + scope :with_artifacts_not_expired, ->() { with_artifacts(:archive).where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } + scope :with_expired_artifacts, ->() { with_artifacts(:archive).where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index a72a815bfe8..0eea6645985 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -514,7 +514,7 @@ module Ci # We purposely cast the builds to an Array here. Because we always use the # rows if there are more than 0 this prevents us from having to run two # queries: one to get the count and one to get the rows. - @latest_builds_with_artifacts ||= builds.latest.with_artifacts.to_a + @latest_builds_with_artifacts ||= builds.latest.with_artifacts(:archive).to_a end private diff --git a/app/models/project.rb b/app/models/project.rb index 0183e3d0a38..d51f7c9e8cd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -542,7 +542,7 @@ class Project < ActiveRecord::Base latest_pipeline = pipelines.latest_successful_for(ref) if latest_pipeline - latest_pipeline.builds.latest.with_artifacts + latest_pipeline.builds.latest.with_artifacts(:archive) else builds.none end -- cgit v1.2.1 From aa91fdff715cbf8d953754fd07fbded0c71e8dab Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 14 Mar 2018 00:31:48 +0900 Subject: Add rubocop:disable GitlabSecurity/PublicSend --- app/models/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 1e2b8e4793d..dafad611060 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -43,7 +43,7 @@ module Ci scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->(file_type) do where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', - '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').public_send(file_type)) + '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').public_send(file_type)) # rubocop:disable GitlabSecurity/PublicSend end scope :with_artifacts_not_expired, ->() { with_artifacts(:archive).where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts(:archive).where('artifacts_expire_at < ?', Time.now) } -- cgit v1.2.1 From b4e8e7ed3aab3a55a6a77b8c6473ad5fafb75ae6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 14 Mar 2018 00:52:34 +0900 Subject: Add feartue spec. Fix failed test --- spec/features/projects/pipelines/pipelines_spec.rb | 12 ++++++++++++ spec/migrations/migrate_old_artifacts_spec.rb | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 33ad59abfdf..99135bfe89d 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -349,6 +349,18 @@ describe 'Pipelines', :js do it { expect(page).not_to have_selector('.build-artifacts') } end + + context 'with trace artifact' do + before do + create(:ci_build, :success, :trace_artifact, pipeline: pipeline) + + visit_project_pipelines + end + + it 'does not show trace artifact as artifacts' + expect(page).not_to have_selector('.build-artifacts') + end + end end context 'mini pipeline graph' do diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb index 92eb1d9ce86..3a6fab6572c 100644 --- a/spec/migrations/migrate_old_artifacts_spec.rb +++ b/spec/migrations/migrate_old_artifacts_spec.rb @@ -66,7 +66,7 @@ describe MigrateOldArtifacts do end it 'all files do have artifacts' do - Ci::Build.with_artifacts do |build| + Ci::Build.with_artifacts(:archive) do |build| expect(build).to have_artifacts end end -- cgit v1.2.1 From c13887a1c219e8ff1a5134d7bd12529cfe656c77 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 14 Mar 2018 16:54:15 +0900 Subject: Fix corrupted test --- spec/features/projects/pipelines/pipelines_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 99135bfe89d..0e81c6c629a 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -357,7 +357,7 @@ describe 'Pipelines', :js do visit_project_pipelines end - it 'does not show trace artifact as artifacts' + it 'does not show trace artifact as artifacts' do expect(page).not_to have_selector('.build-artifacts') end end -- cgit v1.2.1 From 8b0b8e3bc4b1832d7c66e88ec26a3e244a11fec5 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 15 Mar 2018 18:03:37 +0900 Subject: Use with_artifacts_archive name. Add spec --- app/models/ci/build.rb | 8 ++++---- app/models/ci/pipeline.rb | 2 +- app/models/project.rb | 2 +- spec/models/ci/build_spec.rb | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index dafad611060..5a381bdd6a2 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -41,12 +41,12 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } - scope :with_artifacts, ->(file_type) do + scope :with_artifacts_archive, ->() do where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', - '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').public_send(file_type)) # rubocop:disable GitlabSecurity/PublicSend + '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').archive) end - scope :with_artifacts_not_expired, ->() { with_artifacts(:archive).where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } - scope :with_expired_artifacts, ->() { with_artifacts(:archive).where('artifacts_expire_at < ?', Time.now) } + scope :with_artifacts_not_expired, ->() { with_artifacts_archive.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } + scope :with_expired_artifacts, ->() { with_artifacts_archive.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 0eea6645985..8492d293343 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -514,7 +514,7 @@ module Ci # We purposely cast the builds to an Array here. Because we always use the # rows if there are more than 0 this prevents us from having to run two # queries: one to get the count and one to get the rows. - @latest_builds_with_artifacts ||= builds.latest.with_artifacts(:archive).to_a + @latest_builds_with_artifacts ||= builds.latest.with_artifacts_archive.to_a end private diff --git a/app/models/project.rb b/app/models/project.rb index d51f7c9e8cd..8ed236e5c98 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -542,7 +542,7 @@ class Project < ActiveRecord::Base latest_pipeline = pipelines.latest_successful_for(ref) if latest_pipeline - latest_pipeline.builds.latest.with_artifacts(:archive) + latest_pipeline.builds.latest.with_artifacts_archive else builds.none end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 6e202de0db9..623b4d63db4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -80,6 +80,42 @@ describe Ci::Build do end end + describe '.with_artifacts_archive' do + subject { described_class.with_artifacts_archive } + + context 'when job does not have an archive' do + let!(:job) { create(:ci_build) } + + it 'does not return the job' do + is_expected.not_to include(job) + end + end + + context 'when job has a legacy archive' do + let!(:job) { create(:ci_build, :legacy_artifacts) } + + it 'returns the job' do + is_expected.to include(job) + end + end + + context 'when job has a job artifact archive' do + let!(:job) { create(:ci_build, :artifacts) } + + it 'returns the job' do + is_expected.to include(job) + end + end + + context 'when job has a job artifact trace' do + let!(:job) { create(:ci_build, :trace_artifact) } + + it 'does not return the job' do + is_expected.not_to include(job) + end + end + end + describe '#actionize' do context 'when build is a created' do before do -- cgit v1.2.1 From 63a1c74c59142ad39a93d6089dbd30c7f489367d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 16 Mar 2018 00:11:49 +0900 Subject: Fix spec --- spec/migrations/migrate_old_artifacts_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/migrations/migrate_old_artifacts_spec.rb b/spec/migrations/migrate_old_artifacts_spec.rb index 3a6fab6572c..638b2853374 100644 --- a/spec/migrations/migrate_old_artifacts_spec.rb +++ b/spec/migrations/migrate_old_artifacts_spec.rb @@ -66,7 +66,7 @@ describe MigrateOldArtifacts do end it 'all files do have artifacts' do - Ci::Build.with_artifacts(:archive) do |build| + Ci::Build.with_artifacts_archive do |build| expect(build).to have_artifacts end end -- cgit v1.2.1