Commit 7383453b authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'ci/improve-ci-build-api' into 'master'

Improve CI builds API specs

This modifies a CI Runners Builds API, to improve performance, and add few missing examples.
Extracted from !2560 (cherry-picked + extended).

cc @ayufan 

See merge request !2698
parents 76fbb42a bf6d6948
# == Schema Information
#
# Table name: builds
#
# id :integer not null, primary key
# project_id :integer
# status :string(255)
# finished_at :datetime
# trace :text
# created_at :datetime
# updated_at :datetime
# started_at :datetime
# runner_id :integer
# commit_id :integer
# coverage :float
# commands :text
# job_id :integer
# name :string(255)
# deploy :boolean default(FALSE)
# options :text
# allow_failure :boolean default(FALSE), not null
# stage :string(255)
# trigger_request_id :integer
#
# Read about factories at https://github.com/thoughtbot/factory_girl
FactoryGirl.define do FactoryGirl.define do
factory :ci_build, class: Ci::Build do factory :ci_build, class: Ci::Build do
name 'test' name 'test'
...@@ -65,5 +38,20 @@ FactoryGirl.define do ...@@ -65,5 +38,20 @@ FactoryGirl.define do
build.trace = 'BUILD TRACE' build.trace = 'BUILD TRACE'
end end
end end
trait :artifacts do
after(:create) do |build, _|
build.artifacts_file =
fixture_file_upload(Rails.root +
'spec/fixtures/ci_build_artifacts.zip',
'application/zip')
build.artifacts_metadata =
fixture_file_upload(Rails.root +
'spec/fixtures/ci_build_artifacts_metadata.gz',
'application/x-gzip')
build.save!
end
end
end end
end end
...@@ -151,8 +151,8 @@ describe Ci::API::API do ...@@ -151,8 +151,8 @@ describe Ci::API::API do
context "Artifacts" do context "Artifacts" do
let(:file_upload) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } let(:file_upload) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
let(:file_upload2) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif') } let(:file_upload2) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif') }
let(:commit) { FactoryGirl.create(:ci_commit, project: project) } let(:commit) { create(:ci_commit, project: project) }
let(:build) { FactoryGirl.create(:ci_build, commit: commit, runner_id: runner.id) } let(:build) { create(:ci_build, commit: commit, runner_id: runner.id) }
let(:authorize_url) { ci_api("/builds/#{build.id}/artifacts/authorize") } let(:authorize_url) { ci_api("/builds/#{build.id}/artifacts/authorize") }
let(:post_url) { ci_api("/builds/#{build.id}/artifacts") } let(:post_url) { ci_api("/builds/#{build.id}/artifacts") }
let(:delete_url) { ci_api("/builds/#{build.id}/artifacts") } let(:delete_url) { ci_api("/builds/#{build.id}/artifacts") }
...@@ -160,12 +160,10 @@ describe Ci::API::API do ...@@ -160,12 +160,10 @@ describe Ci::API::API do
let(:headers) { { "GitLab-Workhorse" => "1.0" } } let(:headers) { { "GitLab-Workhorse" => "1.0" } }
let(:headers_with_token) { headers.merge(Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token) } let(:headers_with_token) { headers.merge(Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token) }
before { build.run! }
describe "POST /builds/:id/artifacts/authorize" do describe "POST /builds/:id/artifacts/authorize" do
context "should authorize posting artifact to running build" do context "should authorize posting artifact to running build" do
before do
build.run!
end
it "using token as parameter" do it "using token as parameter" do
post authorize_url, { token: build.token }, headers post authorize_url, { token: build.token }, headers
expect(response.status).to eq(200) expect(response.status).to eq(200)
...@@ -180,10 +178,6 @@ describe Ci::API::API do ...@@ -180,10 +178,6 @@ describe Ci::API::API do
end end
context "should fail to post too large artifact" do context "should fail to post too large artifact" do
before do
build.run!
end
it "using token as parameter" do it "using token as parameter" do
stub_application_setting(max_artifacts_size: 0) stub_application_setting(max_artifacts_size: 0)
post authorize_url, { token: build.token, filesize: 100 }, headers post authorize_url, { token: build.token, filesize: 100 }, headers
...@@ -197,8 +191,8 @@ describe Ci::API::API do ...@@ -197,8 +191,8 @@ describe Ci::API::API do
end end
end end
context "should get denied" do context 'token is invalid' do
it do it 'should respond with forbidden'do
post authorize_url, { token: 'invalid', filesize: 100 } post authorize_url, { token: 'invalid', filesize: 100 }
expect(response.status).to eq(403) expect(response.status).to eq(403)
end end
...@@ -206,17 +200,13 @@ describe Ci::API::API do ...@@ -206,17 +200,13 @@ describe Ci::API::API do
end end
describe "POST /builds/:id/artifacts" do describe "POST /builds/:id/artifacts" do
context "Disable sanitizer" do context "disable sanitizer" do
before do before do
# by configuring this path we allow to pass temp file from any path # by configuring this path we allow to pass temp file from any path
allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/') allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
end end
context "should post artifact to running build" do context "should post artifact to running build" do
before do
build.run!
end
it "uses regual file post" do it "uses regual file post" do
upload_artifacts(file_upload, headers_with_token, false) upload_artifacts(file_upload, headers_with_token, false)
expect(response.status).to eq(201) expect(response.status).to eq(201)
...@@ -244,10 +234,7 @@ describe Ci::API::API do ...@@ -244,10 +234,7 @@ describe Ci::API::API do
let(:stored_artifacts_file) { build.reload.artifacts_file.file } let(:stored_artifacts_file) { build.reload.artifacts_file.file }
let(:stored_metadata_file) { build.reload.artifacts_metadata.file } let(:stored_metadata_file) { build.reload.artifacts_metadata.file }
before do before { post(post_url, post_data, headers_with_token) }
build.run!
post(post_url, post_data, headers_with_token)
end
context 'post data accelerated by workhorse is correct' do context 'post data accelerated by workhorse is correct' do
let(:post_data) do let(:post_data) do
...@@ -257,11 +244,8 @@ describe Ci::API::API do ...@@ -257,11 +244,8 @@ describe Ci::API::API do
'metadata.name' => metadata.original_filename } 'metadata.name' => metadata.original_filename }
end end
it 'responds with valid status' do
expect(response.status).to eq(201)
end
it 'stores artifacts and artifacts metadata' do it 'stores artifacts and artifacts metadata' do
expect(response.status).to eq(201)
expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename) expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename)
expect(stored_metadata_file.original_filename).to eq(metadata.original_filename) expect(stored_metadata_file.original_filename).to eq(metadata.original_filename)
end end
...@@ -282,56 +266,42 @@ describe Ci::API::API do ...@@ -282,56 +266,42 @@ describe Ci::API::API do
end end
end end
context "artifacts file is too large" do
context "should fail to post too large artifact" do it "should fail to post too large artifact" do
before do
build.run!
end
it do
stub_application_setting(max_artifacts_size: 0) stub_application_setting(max_artifacts_size: 0)
upload_artifacts(file_upload, headers_with_token) upload_artifacts(file_upload, headers_with_token)
expect(response.status).to eq(413) expect(response.status).to eq(413)
end end
end end
context "should fail to post artifacts without file" do context "artifacts post request does not contain file" do
before do it "should fail to post artifacts without file" do
build.run!
end
it do
post post_url, {}, headers_with_token post post_url, {}, headers_with_token
expect(response.status).to eq(400) expect(response.status).to eq(400)
end end
end end
context "should fail to post artifacts without GitLab-Workhorse" do context 'GitLab Workhorse is not configured' do
before do it "should fail to post artifacts without GitLab-Workhorse" do
build.run!
end
it do
post post_url, { token: build.token }, {} post post_url, { token: build.token }, {}
expect(response.status).to eq(403) expect(response.status).to eq(403)
end end
end end
end end
context "should fail to post artifacts for outside of tmp path" do context "artifacts are being stored outside of tmp path" do
before do before do
# by configuring this path we allow to pass file from @tmpdir only # by configuring this path we allow to pass file from @tmpdir only
# but all temporary files are stored in system tmp directory # but all temporary files are stored in system tmp directory
@tmpdir = Dir.mktmpdir @tmpdir = Dir.mktmpdir
allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir) allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir)
build.run!
end end
after do after do
FileUtils.remove_entry @tmpdir FileUtils.remove_entry @tmpdir
end end
it do it "should fail to post artifacts for outside of tmp path" do
upload_artifacts(file_upload, headers_with_token) upload_artifacts(file_upload, headers_with_token)
expect(response.status).to eq(400) expect(response.status).to eq(400)
end end
...@@ -349,35 +319,39 @@ describe Ci::API::API do ...@@ -349,35 +319,39 @@ describe Ci::API::API do
end end
end end
describe "DELETE /builds/:id/artifacts" do describe 'DELETE /builds/:id/artifacts' do
before do let(:build) { create(:ci_build, :artifacts) }
build.run! before { delete delete_url, token: build.token }
post delete_url, token: build.token, file: file_upload
end
it "should delete artifact build" do it 'should remove build artifacts' do
build.success
delete delete_url, token: build.token
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(build.artifacts_file.exists?).to be_falsy
expect(build.artifacts_metadata.exists?).to be_falsy
end end
end end
describe "GET /builds/:id/artifacts" do describe 'GET /builds/:id/artifacts' do
before do before { get get_url, token: build.token }
build.run!
context 'build has artifacts' do
let(:build) { create(:ci_build, :artifacts) }
let(:download_headers) do
{ 'Content-Transfer-Encoding'=>'binary',
'Content-Disposition'=>'attachment; filename=ci_build_artifacts.zip' }
end end
it "should download artifact" do it 'should download artifact' do
build.update_attributes(artifacts_file: file_upload)
get get_url, token: build.token
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(response.headers).to include download_headers
end
end end
it "should fail to download if no artifact uploaded" do context 'build does not has artifacts' do
get get_url, token: build.token it 'should respond with not found' do
expect(response.status).to eq(404) expect(response.status).to eq(404)
end end
end end
end 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