Commit 4a2a127b authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'artifacts-from-ref-and-build-name-api' into 'master'

API for downloading latest successful build

## What does this MR do?

Implement parts of #4255, particularly the API.

## Are there points in the code the reviewer needs to double check?

I still made it that `ref` could be either branch, tag, or even SHA with:

``` ruby
# ref can't be HEAD, can only be branch/tag name or SHA
scope :latest_successful_for, ->(ref) do
  table = quoted_table_name
  # TODO: Use `where(ref: ref).or(sha: ref)` in Rails 5
  where("#{table}.ref = ? OR #{table}.sha = ?", ref, ref).
    success.order(id: :desc)
end
```

Because the reasons I put in:

* https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13165543
* https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5142#note_13165921

But if you still think that it's not good to do it this way, I'll drop it and let's think about the other way to satisfy the requirement specified in https://gitlab.com/gitlab-org/gitlab-ce/issues/4255#note_13101233 It could be `status=any` or `sha=DEADBEAF`

## What are the relevant issue numbers?

Part of #4255

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5347
parents 8ed97054 e5b05fe8
...@@ -19,6 +19,7 @@ v 8.10.0 (unreleased) ...@@ -19,6 +19,7 @@ v 8.10.0 (unreleased)
- Add Application Setting to configure default Repository Path for new projects - Add Application Setting to configure default Repository Path for new projects
- Delete award emoji when deleting a user - Delete award emoji when deleting a user
- Remove pinTo from Flash and make inline flash messages look nicer !4854 (winniehell) - Remove pinTo from Flash and make inline flash messages look nicer !4854 (winniehell)
- Add an API for downloading latest successful build from a particular branch or tag !5347
- Add link to profile to commit avatar !5163 (winniehell) - Add link to profile to commit avatar !5163 (winniehell)
- Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
- Align flash messages with left side of page content !4959 (winniehell) - Align flash messages with left side of page content !4959 (winniehell)
......
...@@ -12,7 +12,7 @@ module Ci ...@@ -12,7 +12,7 @@ module Ci
scope :unstarted, ->() { where(runner_id: nil) } scope :unstarted, ->() { where(runner_id: nil) }
scope :ignore_failures, ->() { where(allow_failure: false) } scope :ignore_failures, ->() { where(allow_failure: false) }
scope :with_artifacts, ->() { where.not(artifacts_file: nil) } scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) }
scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) }
scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) }
scope :manual_actions, ->() { where(when: :manual) } scope :manual_actions, ->() { where(when: :manual) }
......
...@@ -20,6 +20,11 @@ module Ci ...@@ -20,6 +20,11 @@ module Ci
after_touch :update_state after_touch :update_state
after_save :keep_around_commits after_save :keep_around_commits
# ref can't be HEAD or SHA, can only be branch/tag name
scope :latest_successful_for, ->(ref = default_branch) do
where(ref: ref).success.order(id: :desc).limit(1)
end
def self.truncate_sha(sha) def self.truncate_sha(sha)
sha[0...8] sha[0...8]
end end
...@@ -237,7 +242,7 @@ module Ci ...@@ -237,7 +242,7 @@ module Ci
def keep_around_commits def keep_around_commits
return unless project return unless project
project.repository.keep_around(self.sha) project.repository.keep_around(self.sha)
project.repository.keep_around(self.before_sha) project.repository.keep_around(self.before_sha)
end end
......
...@@ -16,7 +16,11 @@ class CommitStatus < ActiveRecord::Base ...@@ -16,7 +16,11 @@ class CommitStatus < ActiveRecord::Base
alias_attribute :author, :user alias_attribute :author, :user
scope :latest, -> { where(id: unscope(:select).select('max(id)').group(:name, :commit_id)) } scope :latest, -> do
max_id = unscope(:select).select("max(#{quoted_table_name}.id)")
where(id: max_id.group(:name, :commit_id))
end
scope :retried, -> { where.not(id: latest) } scope :retried, -> { where.not(id: latest) }
scope :ordered, -> { order(:name) } scope :ordered, -> { order(:name) }
scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) } scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) }
......
...@@ -429,6 +429,17 @@ class Project < ActiveRecord::Base ...@@ -429,6 +429,17 @@ class Project < ActiveRecord::Base
repository.commit(ref) repository.commit(ref)
end end
# ref can't be HEAD, can only be branch/tag name or SHA
def latest_successful_builds_for(ref = default_branch)
pipeline = pipelines.latest_successful_for(ref).to_sql
join_sql = "INNER JOIN (#{pipeline}) pipelines" +
" ON pipelines.id = #{Ci::Build.quoted_table_name}.commit_id"
builds.joins(join_sql).latest.with_artifacts
# TODO: Whenever we dropped support for MySQL, we could change to:
# pipeline = pipelines.latest_successful_for(ref)
# builds.where(pipeline: pipeline).latest.with_artifacts
end
def merge_base_commit(first_commit_id, second_commit_id) def merge_base_commit(first_commit_id, second_commit_id)
sha = repository.merge_base(first_commit_id, second_commit_id) sha = repository.merge_base(first_commit_id, second_commit_id)
repository.commit(sha) if sha repository.commit(sha) if sha
......
...@@ -52,8 +52,7 @@ module API ...@@ -52,8 +52,7 @@ module API
get ':id/builds/:build_id' do get ':id/builds/:build_id' do
authorize_read_builds! authorize_read_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build
present build, with: Entities::Build, present build, with: Entities::Build,
user_can_download_artifacts: can?(current_user, :read_build, user_project) user_can_download_artifacts: can?(current_user, :read_build, user_project)
...@@ -69,18 +68,27 @@ module API ...@@ -69,18 +68,27 @@ module API
get ':id/builds/:build_id/artifacts' do get ':id/builds/:build_id/artifacts' do
authorize_read_builds! authorize_read_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build
artifacts_file = build.artifacts_file present_artifacts!(build.artifacts_file)
end
unless artifacts_file.file_storage? # Download the artifacts file from ref_name and job
return redirect_to build.artifacts_file.url #
end # Parameters:
# id (required) - The ID of a project
# ref_name (required) - The ref from repository
# job (required) - The name for the build
# Example Request:
# GET /projects/:id/artifacts/:ref_name/download?job=name
get ':id/builds/artifacts/:ref_name/download',
requirements: { ref_name: /.+/ } do
authorize_read_builds!
return not_found! unless artifacts_file.exists? builds = user_project.latest_successful_builds_for(params[:ref_name])
latest_build = builds.find_by!(name: params[:job])
present_file!(artifacts_file.path, artifacts_file.filename) present_artifacts!(latest_build.artifacts_file)
end end
# Get a trace of a specific build of a project # Get a trace of a specific build of a project
...@@ -97,8 +105,7 @@ module API ...@@ -97,8 +105,7 @@ module API
get ':id/builds/:build_id/trace' do get ':id/builds/:build_id/trace' do
authorize_read_builds! authorize_read_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build
header 'Content-Disposition', "infile; filename=\"#{build.id}.log\"" header 'Content-Disposition', "infile; filename=\"#{build.id}.log\""
content_type 'text/plain' content_type 'text/plain'
...@@ -118,8 +125,7 @@ module API ...@@ -118,8 +125,7 @@ module API
post ':id/builds/:build_id/cancel' do post ':id/builds/:build_id/cancel' do
authorize_update_builds! authorize_update_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build
build.cancel build.cancel
...@@ -137,8 +143,7 @@ module API ...@@ -137,8 +143,7 @@ module API
post ':id/builds/:build_id/retry' do post ':id/builds/:build_id/retry' do
authorize_update_builds! authorize_update_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build
return forbidden!('Build is not retryable') unless build.retryable? return forbidden!('Build is not retryable') unless build.retryable?
build = Ci::Build.retry(build, current_user) build = Ci::Build.retry(build, current_user)
...@@ -157,8 +162,7 @@ module API ...@@ -157,8 +162,7 @@ module API
post ':id/builds/:build_id/erase' do post ':id/builds/:build_id/erase' do
authorize_update_builds! authorize_update_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build
return forbidden!('Build is not erasable!') unless build.erasable? return forbidden!('Build is not erasable!') unless build.erasable?
build.erase(erased_by: current_user) build.erase(erased_by: current_user)
...@@ -176,8 +180,8 @@ module API ...@@ -176,8 +180,8 @@ module API
post ':id/builds/:build_id/artifacts/keep' do post ':id/builds/:build_id/artifacts/keep' do
authorize_update_builds! authorize_update_builds!
build = get_build(params[:build_id]) build = get_build!(params[:build_id])
return not_found!(build) unless build && build.artifacts? return not_found!(build) unless build.artifacts?
build.keep_artifacts! build.keep_artifacts!
...@@ -192,6 +196,20 @@ module API ...@@ -192,6 +196,20 @@ module API
user_project.builds.find_by(id: id.to_i) user_project.builds.find_by(id: id.to_i)
end end
def get_build!(id)
get_build(id) || not_found!
end
def present_artifacts!(artifacts_file)
if !artifacts_file.file_storage?
redirect_to(build.artifacts_file.url)
elsif artifacts_file.exists?
present_file!(artifacts_file.path, artifacts_file.filename)
else
not_found!
end
end
def filter_builds(builds, scope) def filter_builds(builds, scope)
return builds if scope.nil? || scope.empty? return builds if scope.nil? || scope.empty?
......
...@@ -5,7 +5,9 @@ describe Ci::Build, models: true do ...@@ -5,7 +5,9 @@ describe Ci::Build, models: true do
let(:pipeline) do let(:pipeline) do
create(:ci_pipeline, project: project, create(:ci_pipeline, project: project,
sha: project.commit.id) sha: project.commit.id,
ref: project.default_branch,
status: 'success')
end end
let(:build) { create(:ci_build, pipeline: pipeline) } let(:build) { create(:ci_build, pipeline: pipeline) }
...@@ -720,7 +722,7 @@ describe Ci::Build, models: true do ...@@ -720,7 +722,7 @@ describe Ci::Build, models: true do
describe '#erasable?' do describe '#erasable?' do
subject { build.erasable? } subject { build.erasable? }
it { is_expected.to eq true } it { is_expected.to be_truthy }
end end
describe '#erased?' do describe '#erased?' do
...@@ -728,7 +730,7 @@ describe Ci::Build, models: true do ...@@ -728,7 +730,7 @@ describe Ci::Build, models: true do
subject { build.erased? } subject { build.erased? }
context 'build has not been erased' do context 'build has not been erased' do
it { is_expected.to be false } it { is_expected.to be_falsey }
end end
context 'build has been erased' do context 'build has been erased' do
...@@ -736,12 +738,13 @@ describe Ci::Build, models: true do ...@@ -736,12 +738,13 @@ describe Ci::Build, models: true do
build.erase build.erase
end end
it { is_expected.to be true } it { is_expected.to be_truthy }
end end
end end
context 'metadata and build trace are not available' do context 'metadata and build trace are not available' do
let!(:build) { create(:ci_build, :success, :artifacts) } let!(:build) { create(:ci_build, :success, :artifacts) }
before do before do
build.remove_artifacts_metadata! build.remove_artifacts_metadata!
end end
...@@ -763,19 +766,19 @@ describe Ci::Build, models: true do ...@@ -763,19 +766,19 @@ describe Ci::Build, models: true do
describe '#retryable?' do describe '#retryable?' do
context 'when build is running' do context 'when build is running' do
before { build.run! } before do
build.run!
it 'should return false' do
expect(build.retryable?).to be false
end end
it { expect(build).not_to be_retryable }
end end
context 'when build is finished' do context 'when build is finished' do
before { build.success! } before do
build.success!
it 'should return true' do
expect(build.retryable?).to be true
end end
it { expect(build).to be_retryable }
end end
end end
......
...@@ -377,7 +377,7 @@ describe Project, models: true do ...@@ -377,7 +377,7 @@ describe Project, models: true do
describe '#repository' do describe '#repository' do
let(:project) { create(:project) } let(:project) { create(:project) }
it 'should return valid repo' do it 'returns valid repo' do
expect(project.repository).to be_kind_of(Repository) expect(project.repository).to be_kind_of(Repository)
end end
end end
...@@ -1155,6 +1155,85 @@ describe Project, models: true do ...@@ -1155,6 +1155,85 @@ describe Project, models: true do
end end
end end
describe '#latest_successful_builds_for' do
def create_pipeline(status = 'success')
create(:ci_pipeline, project: project,
sha: project.commit.sha,
ref: project.default_branch,
status: status)
end
def create_build(new_pipeline = pipeline, name = 'test')
create(:ci_build, :success, :artifacts,
pipeline: new_pipeline,
status: new_pipeline.status,
name: name)
end
let(:project) { create(:project) }
let(:pipeline) { create_pipeline }
context 'with many builds' do
it 'gives the latest builds from latest pipeline' do
pipeline1 = create_pipeline
pipeline2 = create_pipeline
build1_p2 = create_build(pipeline2, 'test')
create_build(pipeline1, 'test')
create_build(pipeline1, 'test2')
build2_p2 = create_build(pipeline2, 'test2')
latest_builds = project.latest_successful_builds_for
expect(latest_builds).to contain_exactly(build2_p2, build1_p2)
end
end
context 'with succeeded pipeline' do
let!(:build) { create_build }
context 'standalone pipeline' do
it 'returns builds for ref for default_branch' do
builds = project.latest_successful_builds_for
expect(builds).to contain_exactly(build)
end
it 'returns empty relation if the build cannot be found' do
builds = project.latest_successful_builds_for('TAIL')
expect(builds).to be_kind_of(ActiveRecord::Relation)
expect(builds).to be_empty
end
end
context 'with some pending pipeline' do
before do
create_build(create_pipeline('pending'))
end
it 'gives the latest build from latest pipeline' do
latest_build = project.latest_successful_builds_for
expect(latest_build).to contain_exactly(build)
end
end
end
context 'with pending pipeline' do
before do
pipeline.update(status: 'pending')
create_build(pipeline)
end
it 'returns empty relation' do
builds = project.latest_successful_builds_for
expect(builds).to be_kind_of(ActiveRecord::Relation)
expect(builds).to be_empty
end
end
end
describe '.where_paths_in' do describe '.where_paths_in' do
context 'without any paths' do context 'without any paths' do
it 'returns an empty relation' do it 'returns an empty relation' do
......
require 'spec_helper' require 'spec_helper'
describe API::API, api: true do describe API::API, api: true do
include ApiHelpers include ApiHelpers
let(:user) { create(:user) } let(:user) { create(:user) }
let(:api_user) { user } let(:api_user) { user }
let(:user2) { create(:user) }
let!(:project) { create(:project, creator_id: user.id) } let!(:project) { create(:project, creator_id: user.id) }
let!(:developer) { create(:project_member, :developer, user: user, project: project) } let!(:developer) { create(:project_member, :developer, user: user, project: project) }
let!(:reporter) { create(:project_member, :reporter, user: user2, project: project) } let(:reporter) { create(:project_member, :reporter, project: project) }
let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id) } let(:guest) { create(:project_member, :guest, project: project) }
let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) }
let!(:build) { create(:ci_build, pipeline: pipeline) } let!(:build) { create(:ci_build, pipeline: pipeline) }
describe 'GET /projects/:id/builds ' do describe 'GET /projects/:id/builds ' do
...@@ -172,10 +172,104 @@ describe API::API, api: true do ...@@ -172,10 +172,104 @@ describe API::API, api: true do
end end
end end
describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
let(:api_user) { reporter.user }
let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
def path_for_ref(ref = pipeline.ref, job = build.name)
api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user)
end
context 'when not logged in' do
let(:api_user) { nil }
before do
get path_for_ref
end
it 'gives 401' do
expect(response).to have_http_status(401)
end
end
context 'when logging as guest' do
let(:api_user) { guest.user }
before do
get path_for_ref
end
it 'gives 403' do
expect(response).to have_http_status(403)
end
end
context 'non-existing build' do
shared_examples 'not found' do
it { expect(response).to have_http_status(:not_found) }
end
context 'has no such ref' do
before do
get path_for_ref('TAIL', build.name)
end
it_behaves_like 'not found'
end
context 'has no such build' do
before do
get path_for_ref(pipeline.ref, 'NOBUILD')
end
it_behaves_like 'not found'
end
end
context 'find proper build' do
shared_examples 'a valid file' do
let(:download_headers) do
{ 'Content-Transfer-Encoding' => 'binary',
'Content-Disposition' =>
"attachment; filename=#{build.artifacts_file.filename}" }
end
it { expect(response).to have_http_status(200) }
it { expect(response.headers).to include(download_headers) }
end
context 'with regular branch' do
before do
pipeline.update(ref: 'master',
sha: project.commit('master').sha)
get path_for_ref('master')
end
it_behaves_like 'a valid file'
end
context 'with branch name containing slash' do
before do
pipeline.update(ref: 'improve/awesome',
sha: project.commit('improve/awesome').sha)
end
before do
get path_for_ref('improve/awesome')
end
it_behaves_like 'a valid file'
end
end
end
describe 'GET /projects/:id/builds/:build_id/trace' do describe 'GET /projects/:id/builds/:build_id/trace' do
let(:build) { create(:ci_build, :trace, pipeline: pipeline) } let(:build) { create(:ci_build, :trace, pipeline: pipeline) }
before { get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user) } before do
get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user)
end
context 'authorized user' do context 'authorized user' do
it 'should return specific build trace' do it 'should return specific build trace' do
...@@ -205,7 +299,7 @@ describe API::API, api: true do ...@@ -205,7 +299,7 @@ describe API::API, api: true do
end end
context 'user without :update_build permission' do context 'user without :update_build permission' do
let(:api_user) { user2 } let(:api_user) { reporter.user }
it 'should not cancel build' do it 'should not cancel build' do
expect(response).to have_http_status(403) expect(response).to have_http_status(403)
...@@ -237,7 +331,7 @@ describe API::API, api: true do ...@@ -237,7 +331,7 @@ describe API::API, api: true do
end end
context 'user without :update_build permission' do context 'user without :update_build permission' do
let(:api_user) { user2 } let(:api_user) { reporter.user }
it 'should not retry build' do it 'should not retry build' do
expect(response).to have_http_status(403) expect(response).to have_http_status(403)
......
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