Merge branch 'ci/remove-builds' into 'master'

Make it possible to erase build content (artifacts, trace)

This feature makes it possible to remove build content - build artifacts and build traces.

- [x] Remove artifacts
- [x] Remove metadata
- [x] Remove build traces
- [x] Wait for https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/1942 this to be merged
- [x] Fix the permissions after the merge

Closes #3421

See merge request !2560

CHANGELOG

@@ -69,6 +69,7 @@ v 8.5.0 (unreleased)
   - Allow SAML users to login with no previous account without having to allow
     all Omniauth providers to do so.
   - Allow existing users to auto link their SAML credentials by logging in via SAML
+  - Make it possible to erase a build (trace, artifacts) using UI and API
 
 v 8.4.4
   - Update omniauth-saml gem to 1.4.2

app/controllers/projects/builds_controller.rb

@@ -56,6 +56,12 @@ class Projects::BuildsController < Projects::ApplicationController
     render json: @build.to_json(only: [:status, :id, :sha, :coverage], methods: :sha)
   end
 
+  def erase
+    @build.erase(erased_by: current_user)
+    redirect_to namespace_project_build_path(project.namespace, project, @build),
+                notice: "Build has been sucessfully erased!"
+  end
+
   private
 
   def build

app/models/ci/build.rb

@@ -31,15 +31,19 @@
 #  artifacts_file     :text
 #  gl_project_id      :integer
 #  artifacts_metadata :text
+#  erased_by_id       :integer
+#  erased_at          :datetime
 #
 
 module Ci
   class Build < CommitStatus
     include Gitlab::Application.routes.url_helpers
+
     LAZY_ATTRIBUTES = ['trace']
 
     belongs_to :runner, class_name: 'Ci::Runner'
     belongs_to :trigger_request, class_name: 'Ci::TriggerRequest'
+    belongs_to :erased_by, class_name: 'User'
 
     serialize :options
 
@@ -203,6 +207,10 @@ module Ci
       end
     end
 
+    def has_trace?
+      raw_trace.present?
+    end
+
     def raw_trace
       if File.file?(path_to_trace)
         File.read(path_to_trace)
@@ -359,6 +367,33 @@ module Ci
       Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path, **options).to_entry
     end
 
+    def erase(opts = {})
+      return false unless erasable?
+
+      remove_artifacts_file!
+      remove_artifacts_metadata!
+      erase_trace!
+      update_erased!(opts[:erased_by])
+    end
+
+    def erasable?
+      complete? && (artifacts? || has_trace?)
+    end
+
+    def erased?
+      !self.erased_at.nil?
+    end
+
+    private
+
+    def erase_trace!
+      self.trace = nil
+    end
+
+    def update_erased!(user = nil)
+      self.update(erased_by: user, erased_at: Time.now)
+    end
+
     private
 
     def yaml_variables

app/views/projects/builds/show.html.haml

@@ -76,10 +76,16 @@
         = link_to '#down-build-trace', class: 'btn' do
           %i.fa.fa-angle-down
 
+      - if @build.erased?
+        .erased.alert.alert-warning
+          - erased_by = "by #{link_to @build.erased_by.name, user_path(@build.erased_by)}" if @build.erased_by
+          Build has been erased #{erased_by.html_safe} #{time_ago_with_tooltip(@build.erased_at)}
+      - else
         %pre.trace#build-trace
           %code.bash
             = preserve do
               = raw @build.trace_html
+
       %div#down-build-trace
 
     .col-md-3
@@ -94,20 +100,34 @@
           %h4.title Build artifacts
           .center
             .btn-group{ role: :group }
-              = link_to "Download", @build.artifacts_download_url, class: 'btn btn-sm btn-primary'
+              = link_to @build.artifacts_download_url, class: 'btn btn-sm btn-primary' do
+                = icon('download')
+                Download
+
               - if @build.artifacts_metadata?
-                = link_to "Browse", @build.artifacts_browse_url, class: 'btn btn-sm btn-primary'
+                = link_to @build.artifacts_browse_url, class: 'btn btn-sm btn-primary' do
+                  = icon('folder-open')
+                  Browse
 
       .build-widget
         %h4.title
           Build ##{@build.id}
           - if can?(current_user, :update_build, @project)
-            .pull-right
+            .center
+              .btn-group{ role: :group }
                 - if @build.cancel_url
                   = link_to "Cancel", @build.cancel_url, class: 'btn btn-sm btn-danger', method: :post
                 - elsif @build.retry_url
                   = link_to "Retry", @build.retry_url, class: 'btn btn-sm btn-primary', method: :post
 
+                - if @build.erasable?
+                  = link_to erase_namespace_project_build_path(@project.namespace, @project, @build),
+                            class: 'btn btn-sm btn-warning', method: :post,
+                            data: { confirm: 'Are you sure you want to erase this build?' } do
+                    = icon('eraser')
+                    Erase
+
+        .clearfix
           - if @build.duration
             %p
               %span.attr-name Duration:
@@ -119,6 +139,10 @@
             %p
               %span.attr-name Finished:
               #{time_ago_with_tooltip(@build.finished_at)}
+          - if @build.erased_at
+            %p
+              %span.attr-name Erased:
+              #{time_ago_with_tooltip(@build.erased_at)}
           %p
             %span.attr-name Runner:
             - if @build.runner && current_user && current_user.admin

config/routes.rb

@@ -617,6 +617,7 @@ Rails.application.routes.draw do
             get :status
             post :cancel
             post :retry
+            post :erase
           end
 
           resource :artifacts, only: [] do

db/migrate/20160202091601_add_erasable_to_ci_build.rb

+class AddErasableToCiBuild < ActiveRecord::Migration
+  def change
+    add_reference :ci_builds, :erased_by, references: :users, index: true
+    add_column :ci_builds, :erased_at, :datetime
+  end
+end

db/schema.rb

@@ -129,6 +129,8 @@ ActiveRecord::Schema.define(version: 20160209130428) do
     t.text     "artifacts_file"
     t.integer  "gl_project_id"
     t.text     "artifacts_metadata"
+    t.integer  "erased_by_id"
+    t.datetime "erased_at"
   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
@@ -136,6 +138,7 @@ ActiveRecord::Schema.define(version: 20160209130428) do
   add_index "ci_builds", ["commit_id", "type", "name", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_name_and_ref", using: :btree
   add_index "ci_builds", ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref", using: :btree
   add_index "ci_builds", ["commit_id"], name: "index_ci_builds_on_commit_id", using: :btree
+  add_index "ci_builds", ["erased_by_id"], name: "index_ci_builds_on_erased_by_id", using: :btree
   add_index "ci_builds", ["gl_project_id"], name: "index_ci_builds_on_gl_project_id", using: :btree
   add_index "ci_builds", ["project_id", "commit_id"], name: "index_ci_builds_on_project_id_and_commit_id", using: :btree
   add_index "ci_builds", ["project_id"], name: "index_ci_builds_on_project_id", using: :btree

doc/api/builds.md

@@ -338,3 +338,53 @@ Example of response
     "user": null
 }
 ```
+
+## Erase a build
+
+Erase a single build of a project (remove build artifacts and a build trace)
+
+```
+POST /projects/:id/builds/:build_id/erase
+```
+
+Parameters
+
+| Attribute   | Type    | required | Description         |
+|-------------|---------|----------|---------------------|
+| `id`        | integer | yes      | The ID of a project |
+| `build_id`  | integer | yes      | The ID of a build   |
+
+Example of request
+
+```
+curl -X POST -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/builds/1/erase"
+```
+
+Example of response
+
+```json
+{
+    "commit": {
+        "author_email": "admin@example.com",
+        "author_name": "Administrator",
+        "created_at": "2015-12-24T16:51:14.000+01:00",
+        "id": "0ff3ae198f8601a285adcf5c0fff204ee6fba5fd",
+        "message": "Test the CI integration.",
+        "short_id": "0ff3ae19",
+        "title": "Test the CI integration."
+    },
+    "coverage": null,
+    "download_url": null,
+    "id": 69,
+    "name": "rubocop",
+    "ref": "master",
+    "runner": null,
+    "stage": "test",
+    "created_at": "2016-01-11T10:13:33.506Z",
+    "started_at": "2016-01-11T10:13:33.506Z",
+    "finished_at": "2016-01-11T10:15:10.506Z",
+    "status": "failed",
+    "tag": false,
+    "user": null
+}
+```

features/project/builds/summary.feature

@@ -13,3 +13,12 @@ Feature: Project Builds Summary
   Scenario: I browse project builds page
     When I visit project builds page
     Then I see button to CI Lint
+
+  Scenario: I erase a build
+    Given recent build is successful
+    And recent build has a build trace
+    When I visit recent build details page
+    And I click erase build button
+    Then recent build has been erased
+    And recent build summary does not have artifacts widget
+    And recent build summary contains information saying that build has been erased

features/steps/project/builds/summary.rb

@@ -10,4 +10,24 @@ class Spinach::Features::ProjectBuildsSummary < Spinach::FeatureSteps
       expect(ci_lint_tool_link[:href]).to eq ci_lint_path
     end
   end
+
+  step 'I click erase build button' do
+    click_link 'Erase'
+  end
+
+  step 'recent build has been erased' do
+    expect(@build.artifacts_file.exists?).to be_falsy
+    expect(@build.artifacts_metadata.exists?).to be_falsy
+    expect(@build.trace).to be_empty
+  end
+
+  step 'recent build summary does not have artifacts widget' do
+    expect(page).to have_no_css('.artifacts')
+  end
+
+  step 'recent build summary contains information saying that build has been erased' do
+    page.within('.erased') do
+      expect(page).to have_content 'Build has been erased'
+    end
+  end
 end

features/steps/shared/builds.rb

@@ -42,6 +42,10 @@ module SharedBuilds
     @build.update_attributes(artifacts_metadata: gzip)
   end
 
+  step 'recent build has a build trace' do
+    @build.trace = 'build trace'
+  end
+
   step 'download of build artifacts archive starts' do
     expect(page.response_headers['Content-Type']).to eq 'application/zip'
     expect(page.response_headers['Content-Transfer-Encoding']).to eq 'binary'

lib/api/builds.rb

@@ -115,13 +115,33 @@ module API
         authorize_update_builds!
 
         build = get_build(params[:build_id])
-        return forbidden!('Build is not retryable') unless build && build.retryable?
+        return not_found!(build) unless build
+        return forbidden!('Build is not retryable') unless build.retryable?
 
         build = Ci::Build.retry(build)
 
         present build, with: Entities::Build,
                        user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
+
+      # Erase build (remove artifacts and build trace)
+      #
+      # Parameters:
+      #   id (required) - the id of a project
+      #   build_id (required) - the id of a build
+      # example Request:
+      #  post  /projects/:id/build/:build_id/erase
+      post ':id/builds/:build_id/erase' do
+        authorize_update_builds!
+
+        build = get_build(params[:build_id])
+        return not_found!(build) unless build
+        return forbidden!('Build is not erasable!') unless build.erasable?
+
+        build.erase(erased_by: current_user)
+        present build, with: Entities::Build,
+                       user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
+      end
     end
 
     helpers do

lib/ci/api/builds.rb

@@ -38,6 +38,8 @@ module Ci
           authenticate_runner!
           update_runner_last_contact
           build = Ci::Build.where(runner_id: current_runner.id).running.find(params[:id])
+          forbidden!('Build has been erased!') if build.erased?
+
           build.update_attributes(trace: params[:trace]) if params[:trace]
 
           case params[:state].to_s
@@ -99,6 +101,7 @@ module Ci
           not_found! unless build
           authenticate_build_token!(build)
           forbidden!('Build is not running!') unless build.running?
+          forbidden!('Build has been erased!') if build.erased?
 
           artifacts_upload_path = ArtifactUploader.artifacts_upload_path
           artifacts = uploaded_file(:file, artifacts_upload_path)
@@ -143,7 +146,7 @@ module Ci
           present_file!(artifacts_file.path, artifacts_file.filename)
         end
 
-        # Remove the artifacts file from build
+        # Remove the artifacts file from build - Runners only
         #
         # Parameters:
         #   id (required) - The ID of a build
@@ -156,6 +159,7 @@ module Ci
           build = Ci::Build.find_by_id(params[:id])
           not_found! unless build
           authenticate_build_token!(build)
+
           build.remove_artifacts_file!
           build.remove_artifacts_metadata!
         end

spec/factories/ci/builds.rb

@@ -62,14 +62,13 @@ FactoryGirl.define do
     trait :artifacts do
       after(:create) do |build, _|
         build.artifacts_file =
-          fixture_file_upload(Rails.root +
-                              'spec/fixtures/ci_build_artifacts.zip',
+          fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
                              'application/zip')
 
         build.artifacts_metadata =
-          fixture_file_upload(Rails.root +
-                              'spec/fixtures/ci_build_artifacts_metadata.gz',
+          fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
                              'application/x-gzip')
+        
         build.save!
       end
     end

spec/models/build_spec.rb

@@ -346,15 +346,14 @@ describe Ci::Build, models: true do
   describe :artifacts_download_url do
     subject { build.artifacts_download_url }
 
-    it "should be nil if artifact doesn't exist" do
-      build.update_attributes(artifacts_file: nil)
-      is_expected.to be_nil
+    context 'artifacts file does not exist' do
+      before { build.update_attributes(artifacts_file: nil) }
+      it { is_expected.to be_nil }
     end
 
-    it 'should not be nil if artifact exist' do
-      gif = fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif')
-      build.update_attributes(artifacts_file: gif)
-      is_expected.to_not be_nil
+    context 'artifacts file exists' do
+      let(:build) { create(:ci_build, :artifacts) }
+      it { is_expected.to_not be_nil }
     end
   end
 
@@ -381,11 +380,7 @@ describe Ci::Build, models: true do
     end
 
     context 'artifacts archive exists' do
-      before do
-        gif = fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif')
-        build.update_attributes(artifacts_file: gif)
-      end
-
+      let(:build) { create(:ci_build, :artifacts) }
       it { is_expected.to be_truthy }
     end
   end
@@ -398,16 +393,7 @@ describe Ci::Build, models: true do
     end
 
     context 'artifacts archive is a zip file and metadata exists' do
-      before do
-        fixture_dir = Rails.root + 'spec/fixtures/'
-        archive = fixture_file_upload(fixture_dir + 'ci_build_artifacts.zip',
-                                      'application/zip')
-        metadata = fixture_file_upload(fixture_dir + 'ci_build_artifacts_metadata.gz',
-                                       'application/x-gzip')
-        build.update_attributes(artifacts_file: archive)
-        build.update_attributes(artifacts_metadata: metadata)
-      end
-
+      let(:build) { create(:ci_build, :artifacts) }
       it { is_expected.to be_truthy }
     end
   end
@@ -511,6 +497,103 @@ describe Ci::Build, models: true do
         expect(@build2.merge_request.id).to eq(@merge_request.id)
       end
     end
+  end
+
+  describe 'build erasable' do
+    shared_examples 'erasable' do
+      it 'should remove artifact file' do
+        expect(build.artifacts_file.exists?).to be_falsy
+      end
+
+      it 'should remove artifact metadata file' do
+        expect(build.artifacts_metadata.exists?).to be_falsy
+      end
+
+      it 'should erase build trace in trace file' do
+        expect(build.trace).to be_empty
+      end
+
+      it 'should set erased to true' do
+        expect(build.erased?).to be true
+      end
+
+      it 'should set erase date' do
+        expect(build.erased_at).to_not be_falsy
+      end
+    end
+
+    context 'build is not erasable' do
+      let!(:build) { create(:ci_build) }
+
+      describe '#erase' do
+        subject { build.erase }
+
+        it { is_expected.to be false }
+      end
+
+      describe '#erasable?' do
+        subject { build.erasable? }
+        it { is_expected.to eq false }
+      end
+    end
+
+    context 'build is erasable' do
+      let!(:build) { create(:ci_build_with_trace, :success, :artifacts) }
+
+      describe '#erase' do
+        before { build.erase(erased_by: user) }
+
+        context 'erased by user' do
+          let!(:user) { create(:user, username: 'eraser') }
+
+          include_examples 'erasable'
+
+          it 'should record user who erased a build' do
+            expect(build.erased_by).to eq user
+          end
+        end
+
+        context 'erased by system' do
+          let(:user) { nil }
+
+          include_examples 'erasable'
+
+          it 'should not set user who erased a build' do
+            expect(build.erased_by).to be_nil
+          end
+        end
+      end
 
+      describe '#erasable?' do
+        subject { build.erasable? }
+        it { is_expected.to eq true }
+      end
+
+      describe '#erased?' do
+        let!(:build) { create(:ci_build_with_trace, :success, :artifacts) }
+        subject { build.erased? }
+
+        context 'build has not been erased' do
+          it { is_expected.to be false }
+        end
+
+        context 'build has been erased' do
+          before { build.erase }
+
+          it { is_expected.to be true }
+        end
+      end
+
+      context 'metadata and build trace are not available' do
+        let!(:build) { create(:ci_build, :success, :artifacts) }
+        before { build.remove_artifacts_metadata! }
+
+        describe '#erase' do
+          it 'should not raise error' do
+            expect { build.erase }.to_not raise_error
+          end
+        end
+      end
+    end
   end
 end

spec/requests/api/builds_spec.rb

@@ -169,4 +169,34 @@ describe API::API, api: true  do
       end
     end
   end
+
+  describe 'POST /projects/:id/builds/:build_id/erase' do
+    before do
+      post api("/projects/#{project.id}/builds/#{build.id}/erase", user)
+    end
+
+    context 'build is erasable' do
+      let(:build) { create(:ci_build_with_trace, :artifacts, :success, project: project, commit: commit) }
+
+      it 'should erase build content' do
+        expect(response.status).to eq 201
+        expect(build.trace).to be_empty
+        expect(build.artifacts_file.exists?).to be_falsy
+        expect(build.artifacts_metadata.exists?).to be_falsy
+      end
+
+      it 'should update build' do
+        expect(build.reload.erased_at).to be_truthy
+        expect(build.reload.erased_by).to eq user
+      end
+    end
+
+    context 'build is not erasable' do
+      let(:build) { create(:ci_build_with_trace, project: project, commit: commit) }
+
+      it 'should respond with forbidden' do
+        expect(response.status).to eq 403
+      end
+    end
+  end
 end

spec/requests/ci/api/builds_spec.rb

@@ -131,20 +131,28 @@ describe Ci::API::API do
     end
 
     describe "PUT /builds/:id" do
-      let(:commit) { FactoryGirl.create(:ci_commit, project: project)}
-      let(:build) { FactoryGirl.create(:ci_build, commit: commit, runner_id: runner.id) }
+      let(:commit) {create(:ci_commit, project: project)}
+      let(:build) { create(:ci_build_with_trace, commit: commit, runner_id: runner.id) }
 
-      it "should update a running build" do
+      before do
         build.run!
         put ci_api("/builds/#{build.id}"), token: runner.token
+      end
+
+      it "should update a running build" do
         expect(response.status).to eq(200)
       end
 
-      it 'Should not override trace information when no trace is given' do
-        build.run!
-        build.update!(trace: 'hello_world')
-        put ci_api("/builds/#{build.id}"), token: runner.token
-        expect(build.reload.trace).to eq 'hello_world'
+      it 'should not override trace information when no trace is given' do
+        expect(build.reload.trace).to eq 'BUILD TRACE'
+      end
+
+      context 'build has been erased' do
+        let(:build) { create(:ci_build, runner_id: runner.id, erased_at: Time.now) }
+
+        it 'should respond with forbidden' do
+          expect(response.status).to eq 403
+        end
       end
     end
 
@@ -191,9 +199,10 @@ describe Ci::API::API do
           end
         end
 
-        context 'token is invalid' do
-          it 'should respond with forbidden'do
-            post authorize_url, { token: 'invalid', filesize: 100 }
+        context 'authorization token is invalid' do
+          before { post authorize_url, { token: 'invalid', filesize: 100 } }
+
+          it 'should respond with forbidden' do
             expect(response.status).to eq(403)
           end
         end
@@ -206,6 +215,15 @@ describe Ci::API::API do
             allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
           end
 
+          context 'build has been erased' do
+            let(:build) { create(:ci_build, erased_at: Time.now) }
+            before { upload_artifacts(file_upload, headers_with_token) }
+
+            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)
@@ -234,7 +252,9 @@ describe Ci::API::API do
             let(:stored_artifacts_file) { build.reload.artifacts_file.file }
             let(:stored_metadata_file) { build.reload.artifacts_metadata.file }
 
-            before { post(post_url, post_data, headers_with_token) }
+            before do
+              post(post_url, post_data, headers_with_token)
+            end
 
             context 'post data accelerated by workhorse is correct' do
               let(:post_data) do