Merge branch 'save-artifacts_sizes' into 'master'

Save artifacts sizes

## What does this MR do?

Introduce ci_builds.artifacts_size as an integer, so that it's easier to access than reading from the file again.

## What are the relevant issue numbers?

Closes #18869

See merge request !4964

6 files changed, 69 insertions, 21 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 3b89fa05801..5b91ee1159c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -10,6 +10,7 @@ v 8.10.0 (unreleased)
   - Display last commit of deleted branch in push events !4699 (winniehell)
   - Apply the trusted_proxies config to the rack request object for use with rack_attack
   - Add Sidekiq queue duration to transaction metrics.
+  - Add a new column `artifacts_size` to table `ci_builds` !4964
   - Let Workhorse serve format-patch diffs
   - Make images fit to the size of the viewport !4810
   - Fix check for New Branch button on Issue page !4630 (winniehell)
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index c11f8e6884d..5c973749975 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -19,6 +19,7 @@ module Ci
 
     acts_as_taggable
 
+    before_save :update_artifacts_size, if: :artifacts_file_changed?
     before_destroy { project }
 
     after_create :execute_hooks
@@ -329,7 +330,12 @@ module Ci
     end
 
     def artifacts_metadata_entry(path, **options)
-      Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path, **options).to_entry
+      metadata = Gitlab::Ci::Build::Artifacts::Metadata.new(
+        artifacts_metadata.path,
+        path,
+        **options)
+
+      metadata.to_entry
     end
 
     def erase_artifacts!
@@ -375,6 +381,14 @@ module Ci
 
     private
 
+    def update_artifacts_size
+      self.artifacts_size = if artifacts_file.exists?
+                              artifacts_file.size
+                            else
+                              nil
+                            end
+    end
+
     def erase_trace!
       self.trace = nil
     end
diff --git a/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb b/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb
new file mode 100644
index 00000000000..61dd726fac7
--- /dev/null
+++ b/db/migrate/20160628085157_add_artifacts_size_to_ci_builds.rb
@@ -0,0 +1,7 @@
+class AddArtifactsSizeToCiBuilds < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  def change
+    add_column(:ci_builds, :artifacts_size, :integer)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index beb723c3bc5..0f5f9f243fa 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"
@@ -164,6 +164,7 @@ ActiveRecord::Schema.define(version: 20160620115026) do
     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 9f270f7b387..260ac81f5fa 100644
--- a/lib/ci/api/builds.rb
+++ b/lib/ci/api/builds.rb
@@ -195,8 +195,7 @@ module Ci
           not_found! unless build
           authenticate_build_token!(build)
 
-          build.remove_artifacts_file!
-          build.remove_artifacts_metadata!
+          build.erase_artifacts!
         end
       end
     end
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 1bc51783c3a..e7cbc3dd3a7 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -293,33 +293,52 @@ describe Ci::API::API do
             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 { 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)
+          describe 'uploading artifacts for a running build' do
+            shared_examples 'successful artifacts upload' 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
 
-            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 regular file post' do
+              before do
+                upload_artifacts(file_upload, headers_with_token, false)
+              end
+
+              it_behaves_like 'successful artifacts upload'
             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 'uses accelerated file post' do
+              before do
+                upload_artifacts(file_upload, headers_with_token, true)
+              end
+
+              it_behaves_like 'successful artifacts upload'
+            end
+
+            context 'updates artifact' do
+              before do
+                upload_artifacts(file_upload2, headers_with_token)
+                upload_artifacts(file_upload, headers_with_token)
+              end
+
+              it_behaves_like 'successful artifacts upload'
             end
           end
 
@@ -329,6 +348,7 @@ describe Ci::API::API do
 
             let(:stored_artifacts_file) { build.reload.artifacts_file.file }
             let(:stored_metadata_file) { build.reload.artifacts_metadata.file }
+            let(:stored_artifacts_size) { build.reload.artifacts_size }
 
             before do
               post(post_url, post_data, headers_with_token)
@@ -346,6 +366,7 @@ describe Ci::API::API do
                 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_size).to eq(71759)
               end
             end
 
@@ -455,12 +476,17 @@ describe Ci::API::API do
 
       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_nil
         end
       end