diff options
author | Olivier Gonzalez <ogonzalez@gitlab.com> | 2018-09-28 00:15:08 +0300 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2018-09-28 00:15:08 +0300 |
commit | cfedc0a9f4732afbf39fdf96e9b6a8598faeba5f (patch) | |
tree | 001b6eb5fd448bc6389842bbe1d03b8587a6b55b | |
parent | 79498893832db7a88e07d8f3e7a629944c80705b (diff) |
Extend reports to support security features
18 files changed, 379 insertions, 203 deletions
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 63aaa0f7bcc..3dadb95443a 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -68,8 +68,12 @@ module Ci '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').archive) end + scope :with_existing_job_artifacts, ->(query) do + where('EXISTS (?)', ::Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').merge(query)) + end + scope :with_archived_trace, ->() do - where('EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').trace) + with_existing_job_artifacts(Ci::JobArtifact.trace) end scope :without_archived_trace, ->() do @@ -77,10 +81,12 @@ module Ci end scope :with_test_reports, ->() do - includes(:job_artifacts_junit) # Prevent N+1 problem when iterating each ci_job_artifact row - .where('EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').test_reports) + with_existing_job_artifacts(Ci::JobArtifact.test_reports) + .eager_load_job_artifacts end + scope :eager_load_job_artifacts, -> { includes(:job_artifacts) } + scope :with_artifacts_stored_locally, -> { with_artifacts_archive.where(artifacts_file_store: [nil, LegacyArtifactUploader::Store::LOCAL]) } scope :with_archived_trace_stored_locally, -> { with_archived_trace.where(artifacts_file_store: [nil, LegacyArtifactUploader::Store::LOCAL]) } scope :with_artifacts_not_expired, ->() { with_artifacts_archive.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } @@ -404,8 +410,8 @@ module Ci trace.exist? end - def has_test_reports? - job_artifacts.test_reports.any? + def has_job_artifacts? + job_artifacts.any? end def has_old_trace? @@ -469,28 +475,23 @@ module Ci end end - def erase_artifacts! - remove_artifacts_file! - remove_artifacts_metadata! - save - end - - def erase_test_reports! - # TODO: Use fast_destroy_all in the context of https://gitlab.com/gitlab-org/gitlab-ce/issues/35240 - job_artifacts_junit&.destroy + # and use that for `ExpireBuildInstanceArtifactsWorker`? + def erase_erasable_artifacts! + job_artifacts.erasable.destroy_all # rubocop: disable DestroyAll + erase_old_artifacts! end def erase(opts = {}) return false unless erasable? - erase_artifacts! - erase_test_reports! + job_artifacts.destroy_all # rubocop: disable DestroyAll + erase_old_artifacts! erase_trace! update_erased!(opts[:erased_by]) end def erasable? - complete? && (artifacts? || has_test_reports? || has_trace?) + complete? && (artifacts? || has_job_artifacts? || has_trace?) end def erased? @@ -648,8 +649,8 @@ module Ci def collect_test_reports!(test_reports) test_reports.get_suite(group_name).tap do |test_suite| - each_test_report do |file_type, blob| - Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, test_suite) + each_report(Ci::JobArtifact::TEST_REPORT_FILE_TYPES) do |file_type, blob| + Gitlab::Ci::Parsers::Test.fabricate!(file_type).parse!(blob, test_suite) end end end @@ -669,6 +670,13 @@ module Ci private + def erase_old_artifacts! + # TODO: To be removed once we get rid of + remove_artifacts_file! + remove_artifacts_metadata! + save + end + def successful_deployment_status if success? && last_deployment&.last? return :last @@ -679,14 +687,19 @@ module Ci :creating end - def each_test_report - Ci::JobArtifact::TEST_REPORT_FILE_TYPES.each do |file_type| - public_send("job_artifacts_#{file_type}").each_blob do |blob| # rubocop:disable GitlabSecurity/PublicSend - yield file_type, blob + def each_report(report_types) + job_artifacts_for_types(report_types).each do |report_artifact| + report_artifact.each_blob do |blob| + yield report_artifact.file_type, blob end end end + def job_artifacts_for_types(report_types) + # Use select to leverage cached associations and avoid N+1 queries + job_artifacts.select { |artifact| artifact.file_type.in?(report_types) } + end + def update_artifacts_size self.artifacts_size = legacy_artifacts_file&.size end diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 93fc1b145b2..259d85e2fe5 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -9,8 +9,28 @@ module Ci NotSupportedAdapterError = Class.new(StandardError) TEST_REPORT_FILE_TYPES = %w[junit].freeze - DEFAULT_FILE_NAMES = { junit: 'junit.xml' }.freeze - TYPE_AND_FORMAT_PAIRS = { archive: :zip, metadata: :gzip, trace: :raw, junit: :gzip }.freeze + NON_ERASABLE_FILE_TYPES = %w[trace].freeze + DEFAULT_FILE_NAMES = { + archive: nil, + metadata: nil, + trace: nil, + junit: 'junit.xml', + sast: 'gl-sast-report.json', + dependency_scanning: 'gl-dependency-scanning-report.json', + container_scanning: 'gl-container-scanning-report.json', + dast: 'gl-dast-report.json' + }.freeze + + TYPE_AND_FORMAT_PAIRS = { + archive: :zip, + metadata: :gzip, + trace: :raw, + junit: :gzip, + sast: :gzip, + dependency_scanning: :gzip, + container_scanning: :gzip, + dast: :gzip + }.freeze belongs_to :project belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id @@ -27,8 +47,18 @@ module Ci scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } + scope :with_file_types, -> (file_types) do + types = self.file_types.select { |file_type| file_types.include?(file_type) }.values + + where(file_type: types) + end + scope :test_reports, -> do - types = self.file_types.select { |file_type| TEST_REPORT_FILE_TYPES.include?(file_type) }.values + with_file_types(TEST_REPORT_FILE_TYPES) + end + + scope :erasable, -> do + types = self.file_types.reject { |file_type| NON_ERASABLE_FILE_TYPES.include?(file_type) }.values where(file_type: types) end @@ -39,7 +69,11 @@ module Ci archive: 1, metadata: 2, trace: 3, - junit: 4 + junit: 4, + sast: 5, ## EE-specific + dependency_scanning: 6, ## EE-specific + container_scanning: 7, ## EE-specific + dast: 8 ## EE-specific } enum file_format: { diff --git a/app/workers/expire_build_instance_artifacts_worker.rb b/app/workers/expire_build_instance_artifacts_worker.rb index 4fcd1e5bd24..94426dcf921 100644 --- a/app/workers/expire_build_instance_artifacts_worker.rb +++ b/app/workers/expire_build_instance_artifacts_worker.rb @@ -13,7 +13,7 @@ class ExpireBuildInstanceArtifactsWorker return unless build&.project && !build.project.pending_delete Rails.logger.info "Removing artifacts for build #{build.id}..." - build.erase_artifacts! + build.erase_erasable_artifacts! end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/changelogs/unreleased/6717_extend_reports_for_security_products.yml b/changelogs/unreleased/6717_extend_reports_for_security_products.yml new file mode 100644 index 00000000000..184c3212e54 --- /dev/null +++ b/changelogs/unreleased/6717_extend_reports_for_security_products.yml @@ -0,0 +1,5 @@ +--- +title: Extend reports feature to support Security Products +merge_request: 21892 +author: +type: added diff --git a/lib/gitlab/ci/config/entry/reports.rb b/lib/gitlab/ci/config/entry/reports.rb index 5963f3eb90c..7bc482a6f48 100644 --- a/lib/gitlab/ci/config/entry/reports.rb +++ b/lib/gitlab/ci/config/entry/reports.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Gitlab module Ci class Config @@ -9,7 +11,7 @@ module Gitlab include Validatable include Attributable - ALLOWED_KEYS = %i[junit].freeze + ALLOWED_KEYS = %i[junit sast dependency_scanning container_scanning dast].freeze attributes ALLOWED_KEYS @@ -19,6 +21,10 @@ module Gitlab with_options allow_nil: true do validates :junit, array_of_strings_or_string: true + validates :sast, array_of_strings_or_string: true + validates :dependency_scanning, array_of_strings_or_string: true + validates :container_scanning, array_of_strings_or_string: true + validates :dast, array_of_strings_or_string: true end end diff --git a/lib/gitlab/ci/parsers.rb b/lib/gitlab/ci/parsers.rb deleted file mode 100644 index a4eccc08dfc..00000000000 --- a/lib/gitlab/ci/parsers.rb +++ /dev/null @@ -1,9 +0,0 @@ -module Gitlab - module Ci - module Parsers - def self.fabricate!(file_type) - "Gitlab::Ci::Parsers::#{file_type.classify}".constantize.new - end - end - end -end diff --git a/lib/gitlab/ci/parsers/junit.rb b/lib/gitlab/ci/parsers/junit.rb deleted file mode 100644 index d1c136f2009..00000000000 --- a/lib/gitlab/ci/parsers/junit.rb +++ /dev/null @@ -1,66 +0,0 @@ -module Gitlab - module Ci - module Parsers - class Junit - JunitParserError = Class.new(StandardError) - - def parse!(xml_data, test_suite) - root = Hash.from_xml(xml_data) - - all_cases(root) do |test_case| - test_case = create_test_case(test_case) - test_suite.add_test_case(test_case) - end - rescue REXML::ParseException => e - raise JunitParserError, "XML parsing failed: #{e.message}" - rescue => e - raise JunitParserError, "JUnit parsing failed: #{e.message}" - end - - private - - def all_cases(root, parent = nil, &blk) - return unless root.present? - - [root].flatten.compact.map do |node| - next unless node.is_a?(Hash) - - # we allow only one top-level 'testsuites' - all_cases(node['testsuites'], root, &blk) unless parent - - # we require at least one level of testsuites or testsuite - each_case(node['testcase'], &blk) if parent - - # we allow multiple nested 'testsuite' (eg. PHPUnit) - all_cases(node['testsuite'], root, &blk) - end - end - - def each_case(testcase, &blk) - return unless testcase.present? - - [testcase].flatten.compact.map(&blk) - end - - def create_test_case(data) - if data['failure'] - status = ::Gitlab::Ci::Reports::TestCase::STATUS_FAILED - system_output = data['failure'] - else - status = ::Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS - system_output = nil - end - - ::Gitlab::Ci::Reports::TestCase.new( - classname: data['classname'], - name: data['name'], - file: data['file'], - execution_time: data['time'], - status: status, - system_output: system_output - ) - end - end - end - end -end diff --git a/lib/gitlab/ci/parsers/test.rb b/lib/gitlab/ci/parsers/test.rb new file mode 100644 index 00000000000..c6bc9662b07 --- /dev/null +++ b/lib/gitlab/ci/parsers/test.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Parsers + module Test + ParserNotFoundError = Class.new(StandardError) + + PARSERS = { + junit: ::Gitlab::Ci::Parsers::Test::Junit + }.freeze + + def self.fabricate!(file_type) + PARSERS.fetch(file_type.to_sym).new + rescue KeyError + raise ParserNotFoundError, "Cannot find any parser matching file type '#{file_type}'" + end + end + end + end +end diff --git a/lib/gitlab/ci/parsers/test/junit.rb b/lib/gitlab/ci/parsers/test/junit.rb new file mode 100644 index 00000000000..5d7d9a751d8 --- /dev/null +++ b/lib/gitlab/ci/parsers/test/junit.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Parsers + module Test + class Junit + JunitParserError = Class.new(StandardError) + + def parse!(xml_data, test_suite) + root = Hash.from_xml(xml_data) + + all_cases(root) do |test_case| + test_case = create_test_case(test_case) + test_suite.add_test_case(test_case) + end + rescue REXML::ParseException => e + raise JunitParserError, "XML parsing failed: #{e.message}" + rescue => e + raise JunitParserError, "JUnit parsing failed: #{e.message}" + end + + private + + def all_cases(root, parent = nil, &blk) + return unless root.present? + + [root].flatten.compact.map do |node| + next unless node.is_a?(Hash) + + # we allow only one top-level 'testsuites' + all_cases(node['testsuites'], root, &blk) unless parent + + # we require at least one level of testsuites or testsuite + each_case(node['testcase'], &blk) if parent + + # we allow multiple nested 'testsuite' (eg. PHPUnit) + all_cases(node['testsuite'], root, &blk) + end + end + + def each_case(testcase, &blk) + return unless testcase.present? + + [testcase].flatten.compact.map(&blk) + end + + def create_test_case(data) + if data['failure'] + status = ::Gitlab::Ci::Reports::TestCase::STATUS_FAILED + system_output = data['failure'] + else + status = ::Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS + system_output = nil + end + + ::Gitlab::Ci::Reports::TestCase.new( + classname: data['classname'], + name: data['name'], + file: data['file'], + execution_time: data['time'], + status: status, + system_output: system_output + ) + end + end + end + end + end +end diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index f028803ca74..ee79f0ae5fd 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -14,6 +14,33 @@ FactoryBot.define do artifact.project ||= artifact.job.project end + trait :raw do + file_format :raw + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/trace/sample_trace'), 'text/plain') + end + end + + trait :zip do + file_format :zip + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip') + end + end + + trait :gzip do + file_format :gzip + + after(:build) do |artifact, _| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip') + end + end + trait :archive do file_type :archive file_format :zip diff --git a/spec/lib/gitlab/ci/config/entry/reports_spec.rb b/spec/lib/gitlab/ci/config/entry/reports_spec.rb index b3a3a6bee1d..e5fc36b5d7a 100644 --- a/spec/lib/gitlab/ci/config/entry/reports_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/reports_spec.rb @@ -3,27 +3,53 @@ require 'spec_helper' describe Gitlab::Ci::Config::Entry::Reports do let(:entry) { described_class.new(config) } + describe 'validates ALLOWED_KEYS' do + let(:artifact_file_types) { Ci::JobArtifact.file_types } + + described_class::ALLOWED_KEYS.each do |keyword, _| + it "expects #{keyword} to be an artifact file_type" do + expect(artifact_file_types).to include(keyword) + end + end + end + describe 'validation' do context 'when entry config value is correct' do - let(:config) { { junit: %w[junit.xml] } } + using RSpec::Parameterized::TableSyntax - describe '#value' do - it 'returns artifacs configuration' do - expect(entry.value).to eq config + shared_examples 'a valid entry' do |keyword, file| + describe '#value' do + it 'returns artifacs configuration' do + expect(entry.value).to eq({ "#{keyword}": [file] } ) + end end - end - describe '#valid?' do - it 'is valid' do - expect(entry).to be_valid + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end end end - context 'when value is not array' do - let(:config) { { junit: 'junit.xml' } } + where(:keyword, :file) do + :junit | 'junit.xml' + :sast | 'gl-sast-report.json' + :dependency_scanning | 'gl-dependency-scanning-report.json' + :container_scanning | 'gl-container-scanning-report.json' + :dast | 'gl-dast-report.json' + end + + with_them do + context 'when value is an array' do + let(:config) { { "#{keyword}": [file] } } - it 'converts to array' do - expect(entry.value).to eq({ junit: ['junit.xml'] } ) + it_behaves_like 'a valid entry', params[:keyword], params[:file] + end + + context 'when value is not array' do + let(:config) { { "#{keyword}": file } } + + it_behaves_like 'a valid entry', params[:keyword], params[:file] end end end @@ -31,11 +57,13 @@ describe Gitlab::Ci::Config::Entry::Reports do context 'when entry value is not correct' do describe '#errors' do context 'when value of attribute is invalid' do - let(:config) { { junit: 10 } } + where(key: described_class::ALLOWED_KEYS) do + let(:config) { { "#{key}": 10 } } - it 'reports error' do - expect(entry.errors) - .to include 'reports junit should be an array of strings or a string' + it 'reports error' do + expect(entry.errors) + .to include "reports #{key} should be an array of strings or a string" + end end end diff --git a/spec/lib/gitlab/ci/parsers/junit_spec.rb b/spec/lib/gitlab/ci/parsers/test/junit_spec.rb index 47497f06229..a49402c7398 100644 --- a/spec/lib/gitlab/ci/parsers/junit_spec.rb +++ b/spec/lib/gitlab/ci/parsers/test/junit_spec.rb @@ -1,6 +1,6 @@ require 'fast_spec_helper' -describe Gitlab::Ci::Parsers::Junit do +describe Gitlab::Ci::Parsers::Test::Junit do describe '#parse!' do subject { described_class.new.parse!(junit, test_suite) } diff --git a/spec/lib/gitlab/ci/parsers_spec.rb b/spec/lib/gitlab/ci/parsers/test_spec.rb index 2fa83c4abae..0b85b432677 100644 --- a/spec/lib/gitlab/ci/parsers_spec.rb +++ b/spec/lib/gitlab/ci/parsers/test_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Ci::Parsers do +describe Gitlab::Ci::Parsers::Test do describe '.fabricate!' do subject { described_class.fabricate!(file_type) } @@ -16,7 +16,7 @@ describe Gitlab::Ci::Parsers do let(:file_type) { 'undefined' } it 'raises an error' do - expect { subject }.to raise_error(NameError) + expect { subject }.to raise_error(Gitlab::Ci::Parsers::Test::ParserNotFoundError) end end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index dbebda20ce0..e82d93d5935 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -176,9 +176,7 @@ describe Ci::Build do it 'does not execute a query for selecting job artifact one by one' do recorded = ActiveRecord::QueryRecorder.new do subject.each do |build| - Ci::JobArtifact::TEST_REPORT_FILE_TYPES.each do |file_type| - build.public_send("job_artifacts_#{file_type}").file.exists? - end + build.job_artifacts.map { |a| a.file.exists? } end end @@ -550,44 +548,22 @@ describe Ci::Build do end end - describe '#has_test_reports?' do - subject { build.has_test_reports? } + describe '#has_job_artifacts?' do + subject { build.has_job_artifacts? } - context 'when build has a test report' do - let(:build) { create(:ci_build, :test_reports) } + context 'when build has a job artifact' do + let(:build) { create(:ci_build, :artifacts) } it { is_expected.to be_truthy } end - context 'when build does not have test reports' do - let(:build) { create(:ci_build, :artifacts) } + context 'when build does not have job artifacts' do + let(:build) { create(:ci_build, :legacy_artifacts) } it { is_expected.to be_falsy } end end - describe '#erase_test_reports!' do - subject { build.erase_test_reports! } - - context 'when build has a test report' do - let!(:build) { create(:ci_build, :test_reports) } - - it 'removes a test report' do - subject - - expect(build.has_test_reports?).to be_falsy - end - end - - context 'when build does not have test reports' do - let!(:build) { create(:ci_build, :artifacts) } - - it 'does not erase anything' do - expect { subject }.not_to change { Ci::JobArtifact.count } - end - end - end - describe '#has_old_trace?' do subject { build.has_old_trace? } @@ -850,8 +826,8 @@ describe Ci::Build do expect(build.artifacts_metadata.exists?).to be_falsy end - it 'removes test reports' do - expect(build.job_artifacts.test_reports.count).to eq(0) + it 'removes all job_artifacts' do + expect(build.job_artifacts.count).to eq(0) end it 'erases build trace in trace file' do @@ -1022,6 +998,32 @@ describe Ci::Build do end end + describe '#erase_erasable_artifacts!' do + let!(:build) { create(:ci_build, :success) } + + subject { build.erase_erasable_artifacts! } + + before do + Ci::JobArtifact.file_types.keys.each do |file_type| + create(:ci_job_artifact, job: build, file_type: file_type, file_format: Ci::JobArtifact::TYPE_AND_FORMAT_PAIRS[file_type.to_sym]) + end + end + + it "erases erasable artifacts" do + subject + + expect(build.job_artifacts.erasable).to be_empty + end + + it "keeps non erasable artifacts" do + subject + + Ci::JobArtifact::NON_ERASABLE_FILE_TYPES.each do |file_type| + expect(build.send("job_artifacts_#{file_type}")).not_to be_nil + end + end + end + describe '#first_pending' do let!(:first) { create(:ci_build, pipeline: pipeline, status: 'pending', created_at: Date.yesterday) } let!(:second) { create(:ci_build, pipeline: pipeline, status: 'pending') } @@ -2844,16 +2846,10 @@ describe Ci::Build do end it 'raises an error' do - expect { subject }.to raise_error(Gitlab::Ci::Parsers::Junit::JunitParserError) + expect { subject }.to raise_error(Gitlab::Ci::Parsers::Test::Junit::JunitParserError) end end end - - context 'when build does not have test reports' do - it 'raises an error' do - expect { subject }.to raise_error(NoMethodError) - end - end end describe '#artifacts_metadata_entry' do diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 1bf338f4c70..2f449d617be 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -31,6 +31,22 @@ describe Ci::JobArtifact do end end + describe '.erasable' do + subject { described_class.erasable } + + context 'when there is am erasable artifact' do + let!(:artifact) { create(:ci_job_artifact, :junit) } + + it { is_expected.to eq([artifact]) } + end + + context 'when there are no erasable artifacts' do + let!(:artifact) { create(:ci_job_artifact, :trace) } + + it { is_expected.to be_empty } + end + end + describe 'callbacks' do subject { create(:ci_job_artifact, :archive) } @@ -106,34 +122,46 @@ describe Ci::JobArtifact do describe 'validates file format' do subject { artifact } - context 'when archive type with zip format' do - let(:artifact) { build(:ci_job_artifact, :archive, file_format: :zip) } + described_class::TYPE_AND_FORMAT_PAIRS.except(:trace).each do |file_type, file_format| + context "when #{file_type} type with #{file_format} format" do + let(:artifact) { build(:ci_job_artifact, file_type: file_type, file_format: file_format) } - it { is_expected.to be_valid } - end + it { is_expected.to be_valid } + end - context 'when archive type with gzip format' do - let(:artifact) { build(:ci_job_artifact, :archive, file_format: :gzip) } + context "when #{file_type} type without format specification" do + let(:artifact) { build(:ci_job_artifact, file_type: file_type, file_format: nil) } - it { is_expected.not_to be_valid } - end + it { is_expected.not_to be_valid } + end - context 'when archive type without format specification' do - let(:artifact) { build(:ci_job_artifact, :archive, file_format: nil) } + context "when #{file_type} type with other formats" do + described_class.file_formats.except(file_format).values.each do |other_format| + let(:artifact) { build(:ci_job_artifact, file_type: file_type, file_format: other_format) } - it { is_expected.not_to be_valid } + it { is_expected.not_to be_valid } + end + end end + end - context 'when junit type with zip format' do - let(:artifact) { build(:ci_job_artifact, :junit, file_format: :zip) } + describe 'validates DEFAULT_FILE_NAMES' do + subject { described_class::DEFAULT_FILE_NAMES } - it { is_expected.not_to be_valid } + described_class.file_types.each do |file_type, _| + it "expects #{file_type} to be included" do + is_expected.to include(file_type.to_sym) + end end + end - context 'when junit type with gzip format' do - let(:artifact) { build(:ci_job_artifact, :junit, file_format: :gzip) } + describe 'validates TYPE_AND_FORMAT_PAIRS' do + subject { described_class::TYPE_AND_FORMAT_PAIRS } - it { is_expected.to be_valid } + described_class.file_types.each do |file_type, _| + it "expects #{file_type} to be included" do + expect(described_class.file_formats).to include(subject[file_type.to_sym]) + end end end diff --git a/spec/presenters/ci/build_runner_presenter_spec.rb b/spec/presenters/ci/build_runner_presenter_spec.rb index e7019b990dd..a42d1f3d399 100644 --- a/spec/presenters/ci/build_runner_presenter_spec.rb +++ b/spec/presenters/ci/build_runner_presenter_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' describe Ci::BuildRunnerPresenter do let(:presenter) { described_class.new(build) } let(:archive) { { paths: ['sample.txt'] } } - let(:junit) { { junit: ['junit.xml'] } } let(:archive_expectation) do { @@ -14,16 +13,6 @@ describe Ci::BuildRunnerPresenter do } end - let(:junit_expectation) do - { - name: 'junit.xml', - artifact_type: :junit, - artifact_format: :gzip, - paths: ['junit.xml'], - when: 'always' - } - end - describe '#artifacts' do context "when option contains archive-type artifacts" do let(:build) { create(:ci_build, options: { artifacts: archive } ) } @@ -49,20 +38,44 @@ describe Ci::BuildRunnerPresenter do end end - context "when option has 'junit' keyword" do - let(:build) { create(:ci_build, options: { artifacts: { reports: junit } } ) } + context "with reports" do + Ci::JobArtifact::DEFAULT_FILE_NAMES.each do |file_type, filename| + let(:report) { { "#{file_type}": [filename] } } + let(:build) { create(:ci_build, options: { artifacts: { reports: report } } ) } + + let(:report_expectation) do + { + name: filename, + artifact_type: :"#{file_type}", + artifact_format: :gzip, + paths: [filename], + when: 'always' + } + end - it 'presents correct hash' do - expect(presenter.artifacts.first).to include(junit_expectation) + it 'presents correct hash' do + expect(presenter.artifacts.first).to include(report_expectation) + end end end context "when option has both archive and reports specification" do - let(:build) { create(:ci_build, options: { script: 'echo', artifacts: { **archive, reports: junit } } ) } + let(:report) { { junit: ['junit.xml'] } } + let(:build) { create(:ci_build, options: { script: 'echo', artifacts: { **archive, reports: report } } ) } + + let(:report_expectation) do + { + name: 'junit.xml', + artifact_type: :junit, + artifact_format: :gzip, + paths: ['junit.xml'], + when: 'always' + } + end it 'presents correct hash' do expect(presenter.artifacts.first).to include(archive_expectation) - expect(presenter.artifacts.second).to include(junit_expectation) + expect(presenter.artifacts.second).to include(report_expectation) end context "when archive specifies 'expire_in' keyword" do @@ -70,7 +83,7 @@ describe Ci::BuildRunnerPresenter do it 'inherits expire_in from archive' do expect(presenter.artifacts.first).to include({ **archive_expectation, expire_in: '3 mins 4 sec' }) - expect(presenter.artifacts.second).to include({ **junit_expectation, expire_in: '3 mins 4 sec' }) + expect(presenter.artifacts.second).to include({ **report_expectation, expire_in: '3 mins 4 sec' }) end end end diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 6adbbb40489..8770365c893 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -721,7 +721,7 @@ describe API::Jobs do expect(job.trace.exist?).to be_falsy expect(job.artifacts_file.exists?).to be_falsy expect(job.artifacts_metadata.exists?).to be_falsy - expect(job.has_test_reports?).to be_falsy + expect(job.has_job_artifacts?).to be_falsy end it 'updates job' do diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 18d52082399..951c0b16a68 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -24,7 +24,9 @@ describe Ci::RetryBuildService do artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by erased_at auto_canceled_by job_artifacts job_artifacts_archive - job_artifacts_metadata job_artifacts_trace job_artifacts_junit].freeze + job_artifacts_metadata job_artifacts_trace job_artifacts_junit + job_artifacts_sast job_artifacts_dependency_scanning + job_artifacts_container_scanning job_artifacts_dast].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections @@ -38,9 +40,8 @@ describe Ci::RetryBuildService do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } let(:build) do - create(:ci_build, :failed, :artifacts, :test_reports, :expired, :erased, - :queued, :coverage, :tags, :allowed_to_fail, :on_tag, - :triggered, :trace_artifact, :teardown_environment, + create(:ci_build, :failed, :expired, :erased, :queued, :coverage, :tags, + :allowed_to_fail, :on_tag, :triggered, :teardown_environment, description: 'my-job', stage: 'test', stage_id: stage.id, pipeline: pipeline, auto_canceled_by: another_pipeline) end @@ -50,6 +51,15 @@ describe Ci::RetryBuildService do # can reset one of the fields when assigning another. We plan to deprecate # and remove legacy `stage` column in the future. build.update(stage: 'test', stage_id: stage.id) + + # Make sure we have one instance for every possible job_artifact_X + # associations to check they are correctly rejected on build duplication. + Ci::JobArtifact::TYPE_AND_FORMAT_PAIRS.each do |file_type, file_format| + create(:ci_job_artifact, file_format, + file_type: file_type, job: build, expire_at: build.artifacts_expire_at) + end + + build.reload end describe 'clone accessors' do |