From aa3a3fd12fc7152020ffbe54faa36a4f17eb94b2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 28 Jun 2016 16:46:16 +0800 Subject: [PATCH 01/14] Cleanup the tests a bit in order to extend it --- spec/requests/ci/api/builds_spec.rb | 49 ++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 1bc51783c3ad..ea9849247013 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -295,31 +295,50 @@ def register_builds context 'build has been erased' do let(:build) { create(:ci_build, erased_at: Time.now) } - before { upload_artifacts(file_upload, headers_with_token) } + + before do + upload_artifacts(file_upload, headers_with_token) + end it 'should respond with forbidden' do expect(response.status).to eq 403 end end - context "should post artifact to running build" do - it "uses regual file post" do - upload_artifacts(file_upload, headers_with_token, false) - expect(response).to have_http_status(201) - expect(json_response["artifacts_file"]["filename"]).to eq(file_upload.original_filename) + context 'should post artifact to running build' do + shared_examples 'post artifact' do + it 'updates successfully' do + response_filename = + json_response["artifacts_file"]["filename"] + + expect(response).to have_http_status(201) + expect(response_filename).to eq(file_upload.original_filename) + end + end + + context 'uses regular file post' do + before do + upload_artifacts(file_upload, headers_with_token, false) + end + + it_behaves_like 'post artifact' end - it "uses accelerated file post" do - upload_artifacts(file_upload, headers_with_token, true) - expect(response).to have_http_status(201) - expect(json_response["artifacts_file"]["filename"]).to eq(file_upload.original_filename) + context 'uses accelerated file post' do + before do + upload_artifacts(file_upload, headers_with_token, true) + end + + it_behaves_like 'post artifact' end - it "updates artifact" do - upload_artifacts(file_upload, headers_with_token) - upload_artifacts(file_upload2, headers_with_token) - expect(response).to have_http_status(201) - expect(json_response["artifacts_file"]["filename"]).to eq(file_upload2.original_filename) + context 'updates artifact' do + before do + upload_artifacts(file_upload2, headers_with_token) + upload_artifacts(file_upload, headers_with_token) + end + + it_behaves_like 'post artifact' end end -- GitLab From fe0c59d2e61238e1241be448a37be0e3e702a5ce Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 28 Jun 2016 18:14:21 +0800 Subject: [PATCH 02/14] Introduce ci_builds.artifacts_sizes as JSON: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We store the sizes as a hash from path to bytes like: ``` ruby {'ci_artifacts.txt' => 27, 'other_artifacts_0.1.2/another-subdirectory/banana_sample.gif' => 71759, 'other_artifacts_0.1.2/doc_sample.txt' => 1314, 'rails_sample.jpg' => 35255, 'tests_encoding/utf8 test dir ✓/regular_file_2' => 7} ``` So that it's easier to access than reading gzip file again. --- app/models/ci/build.rb | 21 ++++++++++++++++++- ...085157_add_artifacts_sizes_to_ci_builds.rb | 11 ++++++++++ lib/ci/api/builds.rb | 1 + spec/requests/ci/api/builds_spec.rb | 14 +++++++++++-- 4 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2b0bec33131a..d9544a4a279f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -5,6 +5,7 @@ class Build < CommitStatus belongs_to :erased_by, class_name: 'User' serialize :options + serialize :artifacts_sizes, JSON validates :coverage, numericality: true, allow_blank: true validates_presence_of :ref @@ -328,8 +329,19 @@ def artifacts_metadata? artifacts? && artifacts_metadata.exists? end + def artifacts_metadata_sizes + return unless artifacts_metadata? + + entries = new_artifacts_metadata('', recursive: true).find_entries! + + entries.inject({}) do |result, (path, metadata)| + result[path] = metadata[:size] if metadata[:size] + result + end + end + def artifacts_metadata_entry(path, **options) - Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path, **options).to_entry + new_artifacts_metadata(path, **options).to_entry end def erase_artifacts! @@ -375,6 +387,13 @@ def keep_artifacts! private + def new_artifacts_metadata(path, **options) + Gitlab::Ci::Build::Artifacts::Metadata.new( + artifacts_metadata.path, + path, + **options) + end + def erase_trace! self.trace = nil end diff --git a/db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb b/db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb new file mode 100644 index 000000000000..bad260b83eae --- /dev/null +++ b/db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb @@ -0,0 +1,11 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddArtifactsSizesToCiBuilds < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + def change + # Or :json if under PostgreSQL? + add_column(:ci_builds, :artifacts_sizes, :text) + end +end diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 9f270f7b3875..93eed4496e4c 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -147,6 +147,7 @@ class Builds < Grape::API build.artifacts_file = artifacts build.artifacts_metadata = metadata build.artifacts_expire_in = params['expire_in'] + build.artifacts_sizes = build.artifacts_metadata_sizes if build.save present(build, with: Entities::BuildDetails) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index ea9849247013..fb164c221d02 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -309,7 +309,7 @@ def register_builds shared_examples 'post artifact' do it 'updates successfully' do response_filename = - json_response["artifacts_file"]["filename"] + json_response['artifacts_file']['filename'] expect(response).to have_http_status(201) expect(response_filename).to eq(file_upload.original_filename) @@ -344,10 +344,14 @@ def register_builds context 'should post artifacts file and metadata file' do let!(:artifacts) { file_upload } - let!(:metadata) { file_upload2 } + let!(:metadata) do + fixture_file_upload( + Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz') + end let(:stored_artifacts_file) { build.reload.artifacts_file.file } let(:stored_metadata_file) { build.reload.artifacts_metadata.file } + let(:stored_artifacts_sizes) { build.reload.artifacts_sizes } before do post(post_url, post_data, headers_with_token) @@ -365,6 +369,12 @@ def register_builds expect(response).to have_http_status(201) expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename) expect(stored_metadata_file.original_filename).to eq(metadata.original_filename) + expect(stored_artifacts_sizes).to eq( + 'ci_artifacts.txt' => 27, + 'other_artifacts_0.1.2/another-subdirectory/banana_sample.gif' => 71759, + 'other_artifacts_0.1.2/doc_sample.txt' => 1314, + 'rails_sample.jpg' => 35255, + 'tests_encoding/utf8 test dir ✓/regular_file_2' => 7) end end -- GitLab From 0a294698db01112c407575e690a8a368be6b15f8 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 28 Jun 2016 11:16:48 +0000 Subject: [PATCH 03/14] Just save the size in total rather than individual files Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964#note_12741046 --- app/models/ci/build.rb | 26 +++++-------------- ...085157_add_artifacts_size_to_ci_builds.rb} | 5 ++-- db/schema.rb | 3 ++- lib/ci/api/builds.rb | 2 +- spec/requests/ci/api/builds_spec.rb | 14 +++------- 5 files changed, 14 insertions(+), 36 deletions(-) rename db/migrate/{20160628085157_add_artifacts_sizes_to_ci_builds.rb => 20160628085157_add_artifacts_size_to_ci_builds.rb} (57%) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d9544a4a279f..2588274355b2 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -5,7 +5,6 @@ class Build < CommitStatus belongs_to :erased_by, class_name: 'User' serialize :options - serialize :artifacts_sizes, JSON validates :coverage, numericality: true, allow_blank: true validates_presence_of :ref @@ -329,19 +328,13 @@ def artifacts_metadata? artifacts? && artifacts_metadata.exists? end - def artifacts_metadata_sizes - return unless artifacts_metadata? - - entries = new_artifacts_metadata('', recursive: true).find_entries! - - entries.inject({}) do |result, (path, metadata)| - result[path] = metadata[:size] if metadata[:size] - result - end - end - def artifacts_metadata_entry(path, **options) - new_artifacts_metadata(path, **options).to_entry + metadata = Gitlab::Ci::Build::Artifacts::Metadata.new( + artifacts_metadata.path, + path, + **options) + + metadata.to_entry end def erase_artifacts! @@ -387,13 +380,6 @@ def keep_artifacts! private - def new_artifacts_metadata(path, **options) - Gitlab::Ci::Build::Artifacts::Metadata.new( - artifacts_metadata.path, - path, - **options) - end - def erase_trace! self.trace = nil end diff --git a/db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb b/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb similarity index 57% rename from db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb rename to db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb index bad260b83eae..6e6e9dc31638 100644 --- a/db/migrate/20160628085157_add_artifacts_sizes_to_ci_builds.rb +++ b/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb @@ -1,11 +1,10 @@ # See http://doc.gitlab.com/ce/development/migration_style_guide.html # for more information on how to write migrations for GitLab. -class AddArtifactsSizesToCiBuilds < ActiveRecord::Migration +class AddArtifactsSizeToCiBuilds < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers def change - # Or :json if under PostgreSQL? - add_column(:ci_builds, :artifacts_sizes, :text) + add_column(:ci_builds, :artifacts_size, :integer) end end diff --git a/db/schema.rb b/db/schema.rb index 7a8377f687c0..509a2d30f4d2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160620115026) do +ActiveRecord::Schema.define(version: 20160628085157) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -163,6 +163,7 @@ t.datetime "erased_at" t.string "environment" t.datetime "artifacts_expire_at" + t.integer "artifacts_size" end add_index "ci_builds", ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 93eed4496e4c..7bfcc40a9f12 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -147,7 +147,7 @@ class Builds < Grape::API build.artifacts_file = artifacts build.artifacts_metadata = metadata build.artifacts_expire_in = params['expire_in'] - build.artifacts_sizes = build.artifacts_metadata_sizes + build.artifacts_size = artifacts.size if build.save present(build, with: Entities::BuildDetails) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index fb164c221d02..08ec154dd5de 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -344,14 +344,11 @@ def register_builds context 'should post artifacts file and metadata file' do let!(:artifacts) { file_upload } - let!(:metadata) do - fixture_file_upload( - Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz') - end + let!(:metadata) { file_upload2 } let(:stored_artifacts_file) { build.reload.artifacts_file.file } let(:stored_metadata_file) { build.reload.artifacts_metadata.file } - let(:stored_artifacts_sizes) { build.reload.artifacts_sizes } + let(:stored_artifacts_size) { build.reload.artifacts_size } before do post(post_url, post_data, headers_with_token) @@ -369,12 +366,7 @@ def register_builds expect(response).to have_http_status(201) expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename) expect(stored_metadata_file.original_filename).to eq(metadata.original_filename) - expect(stored_artifacts_sizes).to eq( - 'ci_artifacts.txt' => 27, - 'other_artifacts_0.1.2/another-subdirectory/banana_sample.gif' => 71759, - 'other_artifacts_0.1.2/doc_sample.txt' => 1314, - 'rails_sample.jpg' => 35255, - 'tests_encoding/utf8 test dir ✓/regular_file_2' => 7) + expect(stored_artifacts_size).to eq(71759) end end -- GitLab From de543359580ffdd67113e36fba80b3e1bd2262c2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 28 Jun 2016 20:32:32 +0800 Subject: [PATCH 04/14] Prefer Ci::Build#erase_artifacts! --- lib/ci/api/builds.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 7bfcc40a9f12..f6a8d907066f 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -196,8 +196,7 @@ class Builds < Grape::API not_found! unless build authenticate_build_token!(build) - build.remove_artifacts_file! - build.remove_artifacts_metadata! + build.erase_artifacts! end end end -- GitLab From 1bc0d732f604d7a4a616ba34b8ccbb1987038951 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 28 Jun 2016 20:37:46 +0800 Subject: [PATCH 05/14] Also remove ci_builds.artifacts_size when erased --- app/models/ci/build.rb | 1 + spec/requests/ci/api/builds_spec.rb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2588274355b2..0f8c9511ce1f 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -340,6 +340,7 @@ def artifacts_metadata_entry(path, **options) def erase_artifacts! remove_artifacts_file! remove_artifacts_metadata! + self.artifacts_size = nil save end diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 08ec154dd5de..de1ec8fd40d3 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -482,6 +482,7 @@ def upload_artifacts(file, headers = {}, accelerated = true) expect(response).to have_http_status(200) expect(build.artifacts_file.exists?).to be_falsy expect(build.artifacts_metadata.exists?).to be_falsy + expect(build.artifacts_size).to be_falsy end end -- GitLab From 9d8dca08e440ccb730f20dcd79b3b47aef8aeb2e Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 29 Jun 2016 17:44:39 +0800 Subject: [PATCH 06/14] Use AR callbacks as suggested by: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964#note_12744656 --- app/models/ci/build.rb | 6 +++++- lib/ci/api/builds.rb | 1 - spec/requests/ci/api/builds_spec.rb | 8 ++++++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 0f8c9511ce1f..2079d5a2178b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -19,6 +19,7 @@ class Build < CommitStatus acts_as_taggable + before_save :update_artifacts_size, if: :artifacts_file_changed? before_destroy { project } after_create :execute_hooks @@ -340,7 +341,6 @@ def artifacts_metadata_entry(path, **options) def erase_artifacts! remove_artifacts_file! remove_artifacts_metadata! - self.artifacts_size = nil save end @@ -381,6 +381,10 @@ def keep_artifacts! private + def update_artifacts_size + self.artifacts_size = artifacts_file.size + end + def erase_trace! self.trace = nil end diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index f6a8d907066f..260ac81f5fab 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -147,7 +147,6 @@ class Builds < Grape::API build.artifacts_file = artifacts build.artifacts_metadata = metadata build.artifacts_expire_in = params['expire_in'] - build.artifacts_size = artifacts.size if build.save present(build, with: Entities::BuildDetails) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index de1ec8fd40d3..64cb7dd12d0e 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -476,13 +476,17 @@ def upload_artifacts(file, headers = {}, accelerated = true) describe 'DELETE /builds/:id/artifacts' do let(:build) { create(:ci_build, :artifacts) } - before { delete delete_url, token: build.token } + + before do + delete delete_url, token: build.token + build.reload + end it 'should remove build artifacts' do expect(response).to have_http_status(200) expect(build.artifacts_file.exists?).to be_falsy expect(build.artifacts_metadata.exists?).to be_falsy - expect(build.artifacts_size).to be_falsy + expect(build.artifacts_size).to eq(0) end end -- GitLab From 7b06acea1c13b7fa9067902faaed73c7210f4bb3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 1 Jul 2016 00:00:35 +0800 Subject: [PATCH 07/14] Use nil for non-existing files rather than 0 --- app/models/ci/build.rb | 4 +++- spec/requests/ci/api/builds_spec.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2079d5a2178b..850895845f4b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -382,7 +382,9 @@ def keep_artifacts! private def update_artifacts_size - self.artifacts_size = artifacts_file.size + self.artifacts_size = if artifacts_file.exists? + artifacts_file.size + end end def erase_trace! diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 64cb7dd12d0e..666fdbdd2b54 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -486,7 +486,7 @@ def upload_artifacts(file, headers = {}, accelerated = true) expect(response).to have_http_status(200) expect(build.artifacts_file.exists?).to be_falsy expect(build.artifacts_metadata.exists?).to be_falsy - expect(build.artifacts_size).to eq(0) + expect(build.artifacts_size).to be_nil end end -- GitLab From 9f2101804083ba48fc85efce9846eca2d56474bf Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 1 Jul 2016 14:19:46 +0800 Subject: [PATCH 08/14] Remove migration guide comment: They're accessible after doing `rails g migration` anyway. Though I somehow feel this comment could be useful for someone who's new and just browsing the source. --- db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb | 3 --- 1 file changed, 3 deletions(-) diff --git a/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb b/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb index 6e6e9dc31638..61dd726fac7b 100644 --- a/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb +++ b/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb @@ -1,6 +1,3 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - class AddArtifactsSizeToCiBuilds < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers -- GitLab From ef960286e552007a8c4e039de9ea5b3aa71e4669 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 1 Jul 2016 14:30:39 +0800 Subject: [PATCH 09/14] Rename shared_examples, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964#note_12817406 --- spec/requests/ci/api/builds_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 666fdbdd2b54..0c4e9be96ffd 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -306,7 +306,7 @@ def register_builds end context 'should post artifact to running build' do - shared_examples 'post artifact' do + shared_examples 'artifacts sender' do it 'updates successfully' do response_filename = json_response['artifacts_file']['filename'] @@ -321,7 +321,7 @@ def register_builds upload_artifacts(file_upload, headers_with_token, false) end - it_behaves_like 'post artifact' + it_behaves_like 'artifacts sender' end context 'uses accelerated file post' do @@ -329,7 +329,7 @@ def register_builds upload_artifacts(file_upload, headers_with_token, true) end - it_behaves_like 'post artifact' + it_behaves_like 'artifacts sender' end context 'updates artifact' do @@ -338,7 +338,7 @@ def register_builds upload_artifacts(file_upload, headers_with_token) end - it_behaves_like 'post artifact' + it_behaves_like 'artifacts sender' end end -- GitLab From 7cc6a703f6fdec3aba3dfe857624dc3ad54ad7cf Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 4 Jul 2016 19:04:51 +0800 Subject: [PATCH 10/14] Rename to "successful artifacts upload", feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964#note_12861577 --- spec/requests/ci/api/builds_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 0c4e9be96ffd..6b64a16404f6 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -306,7 +306,7 @@ def register_builds end context 'should post artifact to running build' do - shared_examples 'artifacts sender' do + shared_examples 'successful artifacts upload' do it 'updates successfully' do response_filename = json_response['artifacts_file']['filename'] @@ -321,7 +321,7 @@ def register_builds upload_artifacts(file_upload, headers_with_token, false) end - it_behaves_like 'artifacts sender' + it_behaves_like 'successful artifacts upload' end context 'uses accelerated file post' do @@ -329,7 +329,7 @@ def register_builds upload_artifacts(file_upload, headers_with_token, true) end - it_behaves_like 'artifacts sender' + it_behaves_like 'successful artifacts upload' end context 'updates artifact' do @@ -338,7 +338,7 @@ def register_builds upload_artifacts(file_upload, headers_with_token) end - it_behaves_like 'artifacts sender' + it_behaves_like 'successful artifacts upload' end end -- GitLab From 79d7e71487db07926065f17c0430df2e8a9fc574 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 4 Jul 2016 19:06:34 +0800 Subject: [PATCH 11/14] Use describe rather than context, feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964#note_12861588 --- spec/requests/ci/api/builds_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 6b64a16404f6..89cb60bdd0b5 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -305,7 +305,7 @@ def register_builds end end - context 'should post artifact to running build' do + describe 'uploading artifacts for a running build' do shared_examples 'successful artifacts upload' do it 'updates successfully' do response_filename = -- GitLab From 25dd39f8cfb33c643cef2389b1d9d8f2a4efe915 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 4 Jul 2016 19:09:24 +0800 Subject: [PATCH 12/14] Add a new column `artifacts_size` to table `ci_builds` !4964 --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 6506f49174a3..b2ba9dfed798 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.10.0 (unreleased) - Replace Haml with Hamlit to make view rendering faster. !3666 - Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Add Sidekiq queue duration to transaction metrics. + - Add a new column `artifacts_size` to table `ci_builds` !4964 - Make images fit to the size of the viewport !4810 - Fix check for New Branch button on Issue page !4630 (winniehell) - Fix MR-auto-close text added to description. !4836 -- GitLab From b5a2319764be166c5db1d66d1da1dd608555bb0c Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 4 Jul 2016 22:56:23 +0800 Subject: [PATCH 13/14] Explicitly set to nil when artifacts don't exist: Feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964#note_12867273 --- app/models/ci/build.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 48f888499890..5c9737499755 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -384,6 +384,8 @@ def keep_artifacts! def update_artifacts_size self.artifacts_size = if artifacts_file.exists? artifacts_file.size + else + nil end end -- GitLab From 36a73929df7cb47a428fb04740ee81f497327edb Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 4 Jul 2016 23:05:05 +0800 Subject: [PATCH 14/14] Use describe rather than context for this: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4964/diffs#note_12867416 Guidelines: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/testing.md#general-guidelines > Use `context` to test branching logic. --- spec/requests/ci/api/builds_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 89cb60bdd0b5..e7cbc3dd3a7a 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -293,7 +293,7 @@ def register_builds allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/') end - context 'build has been erased' do + describe 'build has been erased' do let(:build) { create(:ci_build, erased_at: Time.now) } before do -- GitLab