Merge branch 'builds-artifacts-API' into 'master'

Introduce API for serving the artifacts archive

Add API to download build artifacts archive in context of GitLab API

/cc @DouweM @grzesiek 

See merge request !2893

8 files changed, 162 insertions, 51 deletions

diff --git a/CHANGELOG b/CHANGELOG
index b4578592600..f89ded67a3d 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -57,6 +57,7 @@ v 8.5.0 (unreleased)
   - Don't process cross-reference notes from forks
   - Fix: init.d script not working on OS X
   - Faster snippet search
+  - Added API to download build artifacts
   - Title for milestones should be unique (Zeger-Jan van de Weg)
   - Validate correctness of maximum attachment size application setting
   - Replaces "Create merge request" link with one to the "Merge Request" when one exists
diff --git a/doc/api/builds.md b/doc/api/builds.md
index 6977dedfa9e..85c9600a1c7 100644
--- a/doc/api/builds.md
+++ b/doc/api/builds.md
@@ -34,6 +34,10 @@ Example of response
         "coverage": null,
         "created_at": "2015-12-24T15:51:21.802Z",
         "download_url": null,
+        "artifacts_file": {
+          "filename": "artifacts.zip",
+          "size": 1000
+        },
         "finished_at": "2015-12-24T17:54:27.895Z",
         "id": 7,
         "name": "teaspoon",
@@ -72,6 +76,7 @@ Example of response
         "coverage": null,
         "created_at": "2015-12-24T15:51:21.727Z",
         "download_url": null,
+        "artifacts_file": null,
         "finished_at": "2015-12-24T17:54:24.921Z",
         "id": 6,
         "name": "spinach:other",
@@ -135,6 +140,7 @@ Example of response
         "coverage": null,
         "created_at": "2016-01-11T10:13:33.506Z",
         "download_url": null,
+        "artifacts_file": null,
         "finished_at": "2016-01-11T10:14:09.526Z",
         "id": 69,
         "name": "rubocop",
@@ -159,6 +165,7 @@ Example of response
         "coverage": null,
         "created_at": "2015-12-24T15:51:21.957Z",
         "download_url": null,
+        "artifacts_file": null,
         "finished_at": "2015-12-24T17:54:33.913Z",
         "id": 9,
         "name": "brakeman",
@@ -220,6 +227,7 @@ Example of response
     "coverage": null,
     "created_at": "2015-12-24T15:51:21.880Z",
     "download_url": null,
+    "artifacts_file": null,
     "finished_at": "2015-12-24T17:54:31.198Z",
     "id": 8,
     "name": "rubocop",
@@ -247,6 +255,35 @@ Example of response
 }
 ```
 
+## Get build artifacts
+
+Get build artifacts of a project
+
+```
+GET /projects/:id/builds/:build_id/artifacts
+```
+
+### 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 -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/builds/8"
+```
+
+### Response:
+
+
+| Status    | Description                     |
+|-----------|---------------------------------|
+| 200       | Serves the artifacts file       |
+| 404       | Build not found or no artifacts |
+
 ## Cancel a build
 
 Cancel a single build of a project
@@ -280,6 +317,7 @@ Example of response
     "coverage": null,
     "created_at": "2016-01-11T10:13:33.506Z",
     "download_url": null,
+    "artifacts_file": null,
     "finished_at": "2016-01-11T10:14:09.526Z",
     "id": 69,
     "name": "rubocop",
@@ -326,6 +364,7 @@ Example of response
     "coverage": null,
     "created_at": "2016-01-11T10:13:33.506Z",
     "download_url": null,
+    "artifacts_file": null,
     "finished_at": null,
     "id": 69,
     "name": "rubocop",
diff --git a/lib/api/builds.rb b/lib/api/builds.rb
index c058128ae54..2b104f90aa7 100644
--- a/lib/api/builds.rb
+++ b/lib/api/builds.rb
@@ -60,6 +60,30 @@ module API
                        user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
 
+      # Download the artifacts file from build
+      #
+      # Parameters:
+      #   id (required) - The ID of a build
+      #   token (required) - The build authorization token
+      # Example Request:
+      #   GET /projects/:id/builds/:build_id/artifacts
+      get ':id/builds/:build_id/artifacts' do
+        authorize_read_builds!
+
+        build = get_build(params[:build_id])
+        return not_found!(build) unless build
+
+        artifacts_file = build.artifacts_file
+
+        unless artifacts_file.file_storage?
+          return redirect_to build.artifacts_file.url
+        end
+
+        return not_found! unless artifacts_file.exists?
+
+        present_file!(artifacts_file.path, artifacts_file.filename)
+      end
+
       # Get a trace of a specific build of a project
       #
       # Parameters:
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 857705dbf12..a3b5f1eb8d3 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -391,6 +391,10 @@ module API
       end
     end
 
+    class BuildArtifactFile < Grape::Entity
+      expose :filename, :size
+    end
+
     class Build < Grape::Entity
       expose :id, :status, :stage, :name, :ref, :tag, :coverage
       expose :created_at, :started_at, :finished_at
@@ -402,6 +406,7 @@ module API
           repo_obj.artifacts_download_url
         end
       end
+      expose :artifacts_file, using: BuildArtifactFile, if: -> (build, opts) { build.artifacts? }
       expose :commit, with: RepoCommit do |repo_obj, _options|
         if repo_obj.respond_to?(:commit)
           repo_obj.commit.commit_data
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 41497c2ce80..a46466798d6 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -53,7 +53,7 @@ FactoryGirl.define do
       tag true
     end
 
-    factory :ci_build_with_trace do
+    trait :trace do
       after(:create) do |build, evaluator|
         build.trace = 'BUILD TRACE'
       end
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 556527837a7..e3d3d453653 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -538,7 +538,7 @@ describe Ci::Build, models: true do
     end
 
     context 'build is erasable' do
-      let!(:build) { create(:ci_build_with_trace, :success, :artifacts) }
+      let!(:build) { create(:ci_build, :trace, :success, :artifacts) }
 
       describe '#erase' do
         before { build.erase(erased_by: user) }
@@ -570,7 +570,7 @@ describe Ci::Build, models: true do
       end
 
       describe '#erased?' do
-        let!(:build) { create(:ci_build_with_trace, :success, :artifacts) }
+        let!(:build) { create(:ci_build, :trace, :success, :artifacts) }
         subject { build.erased? }
 
         context 'build has not been erased' do
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index adbae5dcad8..175ee861a71 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/spec/requests/api/builds_spec.rb
@@ -4,148 +4,190 @@ describe API::API, api: true  do
   include ApiHelpers
 
   let(:user) { create(:user) }
+  let(:api_user) { user }
   let(:user2) { create(:user) }
   let!(:project) { create(:project, creator_id: user.id) }
   let!(:developer) { create(:project_member, user: user, project: project, access_level: ProjectMember::DEVELOPER) }
   let!(:reporter) { create(:project_member, user: user2, project: project, access_level: ProjectMember::REPORTER) }
   let(:commit) { create(:ci_commit, project: project)}
   let(:build) { create(:ci_build, commit: commit) }
-  let(:build_with_trace) { create(:ci_build_with_trace, commit: commit) }
-  let(:build_canceled) { create(:ci_build, :canceled, commit: commit) }
 
   describe 'GET /projects/:id/builds ' do
+    let(:query) { '' }
+
+    before { get api("/projects/#{project.id}/builds?#{query}", api_user) }
+
     context 'authorized user' do
       it 'should return project builds' do
-        get api("/projects/#{project.id}/builds", user)
-
         expect(response.status).to eq(200)
         expect(json_response).to be_an Array
       end
 
-      it 'should filter project with one scope element' do
-        get api("/projects/#{project.id}/builds?scope=pending", user)
+      context 'filter project with one scope element' do
+        let(:query) { 'scope=pending' }
 
-        expect(response.status).to eq(200)
-        expect(json_response).to be_an Array
+        it do
+          expect(response.status).to eq(200)
+          expect(json_response).to be_an Array
+        end
       end
 
-      it 'should filter project with array of scope elements' do
-        get api("/projects/#{project.id}/builds?scope[0]=pending&scope[1]=running", user)
+      context 'filter project with array of scope elements' do
+        let(:query) { 'scope[0]=pending&scope[1]=running' }
 
-        expect(response.status).to eq(200)
-        expect(json_response).to be_an Array
+        it do
+          expect(response.status).to eq(200)
+          expect(json_response).to be_an Array
+        end
       end
 
-      it 'should respond 400 when scope contains invalid state' do
-        get api("/projects/#{project.id}/builds?scope[0]=pending&scope[1]=unknown_status", user)
+      context 'respond 400 when scope contains invalid state' do
+        let(:query) { 'scope[0]=pending&scope[1]=unknown_status' }
 
-        expect(response.status).to eq(400)
+        it { expect(response.status).to eq(400) }
       end
     end
 
     context 'unauthorized user' do
-      it 'should not return project builds' do
-        get api("/projects/#{project.id}/builds")
+      let(:api_user) { nil }
 
+      it 'should not return project builds' do
         expect(response.status).to eq(401)
       end
     end
   end
 
   describe 'GET /projects/:id/repository/commits/:sha/builds' do
+    before do
+      project.ensure_ci_commit(commit.sha)
+      get api("/projects/#{project.id}/repository/commits/#{commit.sha}/builds", api_user)
+    end
+
     context 'authorized user' do
       it 'should return project builds for specific commit' do
-        project.ensure_ci_commit(commit.sha)
-        get api("/projects/#{project.id}/repository/commits/#{commit.sha}/builds", user)
-
         expect(response.status).to eq(200)
         expect(json_response).to be_an Array
       end
     end
 
     context 'unauthorized user' do
-      it 'should not return project builds' do
-        project.ensure_ci_commit(commit.sha)
-        get api("/projects/#{project.id}/repository/commits/#{commit.sha}/builds")
+      let(:api_user) { nil }
 
+      it 'should not return project builds' do
         expect(response.status).to eq(401)
       end
     end
   end
 
   describe 'GET /projects/:id/builds/:build_id' do
+    before { get api("/projects/#{project.id}/builds/#{build.id}", api_user) }
+
     context 'authorized user' do
       it 'should return specific build data' do
-        get api("/projects/#{project.id}/builds/#{build.id}", user)
-
         expect(response.status).to eq(200)
         expect(json_response['name']).to eq('test')
       end
     end
 
     context 'unauthorized user' do
-      it 'should not return specific build data' do
-        get api("/projects/#{project.id}/builds/#{build.id}")
+      let(:api_user) { nil }
 
+      it 'should not return specific build data' do
         expect(response.status).to eq(401)
       end
     end
   end
 
+  describe 'GET /projects/:id/builds/:build_id/artifacts' do
+    before { get api("/projects/#{project.id}/builds/#{build.id}/artifacts", api_user) }
+
+    context 'build with artifacts' do
+      let(:build) { create(:ci_build, :artifacts, commit: commit) }
+
+      context 'authorized user' do
+        let(:download_headers) do
+          { 'Content-Transfer-Encoding'=>'binary',
+            'Content-Disposition'=>'attachment; filename=ci_build_artifacts.zip' }
+        end
+
+        it 'should return specific build artifacts' do
+          expect(response.status).to eq(200)
+          expect(response.headers).to include(download_headers)
+        end
+      end
+
+      context 'unauthorized user' do
+        let(:api_user) { nil }
+
+        it 'should not return specific build artifacts' do
+          expect(response.status).to eq(401)
+        end
+      end
+    end
+
+    it 'should not return build artifacts if not uploaded' do
+      expect(response.status).to eq(404)
+    end
+  end
+
   describe 'GET /projects/:id/builds/:build_id/trace' do
+    let(:build) { create(:ci_build, :trace, commit: commit) }
+    
+    before { get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user) }
+
     context 'authorized user' do
       it 'should return specific build trace' do
-        get api("/projects/#{project.id}/builds/#{build_with_trace.id}/trace", user)
-
         expect(response.status).to eq(200)
-        expect(response.body).to eq(build_with_trace.trace)
+        expect(response.body).to eq(build.trace)
       end
     end
 
     context 'unauthorized user' do
-      it 'should not return specific build trace' do
-        get api("/projects/#{project.id}/builds/#{build_with_trace.id}/trace")
+      let(:api_user) { nil }
 
+      it 'should not return specific build trace' do
         expect(response.status).to eq(401)
       end
     end
   end
 
   describe 'POST /projects/:id/builds/:build_id/cancel' do
+    before { post api("/projects/#{project.id}/builds/#{build.id}/cancel", api_user) }
+
     context 'authorized user' do
       context 'user with :update_build persmission' do
         it 'should cancel running or pending build' do
-          post api("/projects/#{project.id}/builds/#{build.id}/cancel", user)
-
           expect(response.status).to eq(201)
           expect(project.builds.first.status).to eq('canceled')
         end
       end
 
       context 'user without :update_build permission' do
-        it 'should not cancel build' do
-          post api("/projects/#{project.id}/builds/#{build.id}/cancel", user2)
+        let(:api_user) { user2 }
 
+        it 'should not cancel build' do
           expect(response.status).to eq(403)
         end
       end
     end
 
     context 'unauthorized user' do
-      it 'should not cancel build' do
-        post api("/projects/#{project.id}/builds/#{build.id}/cancel")
+      let(:api_user) { nil }
 
+      it 'should not cancel build' do
         expect(response.status).to eq(401)
       end
     end
   end
 
   describe 'POST /projects/:id/builds/:build_id/retry' do
+    let(:build) { create(:ci_build, :canceled, commit: commit) }
+
+    before { post api("/projects/#{project.id}/builds/#{build.id}/retry", api_user) }
+
     context 'authorized user' do
-      context 'user with :update_build persmission' do
+      context 'user with :update_build permission' do
         it 'should retry non-running build' do
-          post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user)
-
           expect(response.status).to eq(201)
           expect(project.builds.first.status).to eq('canceled')
           expect(json_response['status']).to eq('pending')
@@ -153,18 +195,18 @@ describe API::API, api: true  do
       end
 
       context 'user without :update_build permission' do
-        it 'should not retry build' do
-          post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user2)
+        let(:api_user) { user2 }
 
+        it 'should not retry build' do
           expect(response.status).to eq(403)
         end
       end
     end
 
     context 'unauthorized user' do
-      it 'should not retry build' do
-        post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry")
+      let(:api_user) { nil }
 
+      it 'should not retry build' do
         expect(response.status).to eq(401)
       end
     end
@@ -176,7 +218,7 @@ describe API::API, api: true  do
     end
 
     context 'build is erasable' do
-      let(:build) { create(:ci_build_with_trace, :artifacts, :success, project: project, commit: commit) }
+      let(:build) { create(:ci_build, :trace, :artifacts, :success, project: project, commit: commit) }
 
       it 'should erase build content' do
         expect(response.status).to eq 201
@@ -192,7 +234,7 @@ describe API::API, api: true  do
     end
 
     context 'build is not erasable' do
-      let(:build) { create(:ci_build_with_trace, project: project, commit: commit) }
+      let(:build) { create(:ci_build, :trace, project: project, commit: commit) }
 
       it 'should respond with forbidden' do
         expect(response.status).to eq 403
diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb
index 8eb9881649c..57d7eb927fd 100644
--- a/spec/requests/ci/api/builds_spec.rb
+++ b/spec/requests/ci/api/builds_spec.rb
@@ -132,7 +132,7 @@ describe Ci::API::API do
 
     describe "PUT /builds/:id" do
       let(:commit) {create(:ci_commit, project: project)}
-      let(:build) { create(:ci_build_with_trace, commit: commit, runner_id: runner.id) }
+      let(:build) { create(:ci_build, :trace, commit: commit, runner_id: runner.id) }
 
       before do
         build.run!