diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index ef5e7fcac23..34765020287 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -97,12 +97,12 @@ module Gitlab return if trace_artifact if current_path - File.open(current_path) do |f| - archive_stream!(f) - f.unlink + File.open(current_path) do |stream| + archive_stream!(stream) + FileUtils.rm(current_path) end elsif old_trace - StringIO(old_trace).tap do |stream| + StringIO.new(old_trace, 'rb').tap do |stream| archive_stream!(stream) job.erase_old_trace! end @@ -121,7 +121,8 @@ module Gitlab FileUtils.mkdir_p(temp_dir) Dir.mktmpdir('tmp-trace', temp_dir) do |dir_path| temp_path = File.join(dir_path, "job.log") - size = IO.write(src_stream, temp_path) + FileUtils.touch(temp_path) + size = IO.copy_stream(src_stream, temp_path) raise 'Not all saved' unless size == src_stream.size yield(temp_path) end diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb index 91c9625ba06..e52f5fc39d4 100644 --- a/spec/lib/gitlab/ci/trace_spec.rb +++ b/spec/lib/gitlab/ci/trace_spec.rb @@ -399,4 +399,129 @@ describe Gitlab::Ci::Trace do end end end + + describe '#archive!' do + subject { trace.archive! } + + shared_examples 'archive trace file' do + it do + expect { subject }.to change { Ci::JobArtifact.count }.by(1) + + build.reload + expect(build.trace.exist?).to be_truthy + expect(build.job_artifacts_trace.file.exists?).to be_truthy + expect(build.job_artifacts_trace.file.filename).to eq('job.log') + expect(File.exist?(src_path)).to be_falsy + expect(src_checksum) + .to eq(Digest::SHA256.file(build.job_artifacts_trace.file.path).digest) + end + end + + shared_examples 'source trace file stays intact' do |error:| + it do + expect { subject }.to raise_error(error) + + build.reload + expect(build.trace.exist?).to be_truthy + expect(build.job_artifacts_trace).to be_nil + expect(File.exist?(src_path)).to be_truthy + end + end + + shared_examples 'archive trace in database' do + it do + expect { subject }.to change { Ci::JobArtifact.count }.by(1) + + build.reload + expect(build.trace.exist?).to be_truthy + expect(build.job_artifacts_trace.file.exists?).to be_truthy + expect(build.job_artifacts_trace.file.filename).to eq('job.log') + expect(build.old_trace).to be_nil + expect(src_checksum) + .to eq(Digest::SHA256.file(build.job_artifacts_trace.file.path).digest) + end + end + + shared_examples 'source trace in database stays intact' do |error:| + it do + expect { subject }.to raise_error(error) + + build.reload + expect(build.trace.exist?).to be_truthy + expect(build.job_artifacts_trace).to be_nil + expect(build.old_trace).to eq(trace_content) + end + end + + context 'when job does not have trace artifact' do + context 'when trace file stored in default path' do + let!(:build) { create(:ci_build, :trace_live) } + let!(:src_path) { trace.read { |s| return s.path } } + let!(:src_checksum) { Digest::SHA256.file(src_path).digest } + + it_behaves_like 'archive trace file' + + context 'when failed to create clone file' do + before do + allow_any_instance_of(Gitlab::Ci::Trace) + .to receive(:clone_file!).and_raise('Not all saved') + end + + it_behaves_like 'source trace file stays intact', error: 'Not all saved' + end + + context 'when failed to create job artifact record' do + before do + allow_any_instance_of(Ci::JobArtifact).to receive(:save).and_return(false) + allow_any_instance_of(Ci::JobArtifact).to receive_message_chain(:errors, :full_messages) + .and_return(["Error", 'Error']) + end + + it_behaves_like 'source trace file stays intact', error: ActiveRecord::RecordInvalid + end + end + + context 'when trace stored in database' do + let(:trace_content) { IO.read(expand_fixture_path('trace/sample_trace')) } + let!(:src_checksum) { Digest::SHA256.digest(trace_content) } + + before do + build.update_column(:trace, trace_content) + end + + it_behaves_like 'archive trace in database' + + context 'when failed to create clone file' do + before do + allow_any_instance_of(Gitlab::Ci::Trace) + .to receive(:clone_file!).and_raise('Not all saved') + end + + it_behaves_like 'source trace in database stays intact', error: 'Not all saved' + end + + context 'when failed to create job artifact record' do + before do + allow_any_instance_of(Ci::JobArtifact).to receive(:save).and_return(false) + allow_any_instance_of(Ci::JobArtifact).to receive_message_chain(:errors, :full_messages) + .and_return(["Error", 'Error']) + end + + it_behaves_like 'source trace in database stays intact', error: ActiveRecord::RecordInvalid + end + end + end + + context 'when job has trace artifact' do + before do + create(:ci_job_artifact, :trace, job: build) + end + + it 'does not archive' do + expect_any_instance_of(Gitlab::Ci::Trace).not_to receive(:archive_stream!) + expect { subject }.not_to change { Ci::JobArtifact.count } + expect(build.job_artifacts_trace.file.exists?).to be_truthy + end + end + end end diff --git a/spec/services/ci/create_trace_artifact_service_spec.rb b/spec/services/ci/create_trace_artifact_service_spec.rb deleted file mode 100644 index 8c5e8e438c7..00000000000 --- a/spec/services/ci/create_trace_artifact_service_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -require 'spec_helper' - -describe Ci::CreateTraceArtifactService do - describe '#execute' do - subject { described_class.new(nil, nil).execute(job) } - - context 'when the job does not have trace artifact' do - context 'when the job has a trace file' do - let!(:job) { create(:ci_build, :trace_live) } - let!(:legacy_path) { job.trace.read { |stream| return stream.path } } - let!(:legacy_checksum) { Digest::SHA256.file(legacy_path).hexdigest } - let(:new_path) { job.job_artifacts_trace.file.path } - let(:new_checksum) { Digest::SHA256.file(new_path).hexdigest } - - it { expect(File.exist?(legacy_path)).to be_truthy } - - it 'creates trace artifact' do - expect { subject }.to change { Ci::JobArtifact.count }.by(1) - - expect(File.exist?(legacy_path)).to be_falsy - expect(File.exist?(new_path)).to be_truthy - expect(new_checksum).to eq(legacy_checksum) - expect(job.job_artifacts_trace.file.exists?).to be_truthy - expect(job.job_artifacts_trace.file.filename).to eq('job.log') - end - - context 'when failed to create trace artifact record' do - before do - # When ActiveRecord error happens - allow_any_instance_of(Ci::JobArtifact).to receive(:save).and_return(false) - allow_any_instance_of(Ci::JobArtifact).to receive_message_chain(:errors, :full_messages) - .and_return("Error") - - subject rescue nil - - job.reload - end - - it 'keeps legacy trace and removes trace artifact' do - expect(File.exist?(legacy_path)).to be_truthy - expect(job.job_artifacts_trace).to be_nil - end - end - end - - context 'when the job does not have a trace file' do - let!(:job) { create(:ci_build) } - - it 'does not create trace artifact' do - expect { subject }.not_to change { Ci::JobArtifact.count } - end - end - end - - context 'when the job has already had trace artifact' do - let!(:job) { create(:ci_build, :trace_artifact) } - - it 'does not create trace artifact' do - expect { subject }.not_to change { Ci::JobArtifact.count } - end - end - end -end