Add builds API
References #4264 (closed)
Merge request reports
Activity
@ayufan I need to add cancel/retry build features here, but You can review and send me Your comments. This is the first of API endpoints mentioned by You in the issue - after finishing builds the rest we could do in the same way.
- lib/api/builds.rb 0 → 100644
1 module API 2 # Projects builds API 3 class Builds < Grape::API 4 before { authenticate! } What permissions it should check? I've looked into BuildsController and there are only checks for
:manage_builds
(except:index
,:show
and:status
actions) and:download_build_artifacts
but only for artifact download action. I don't see any checks that should be done to get a list of builds or traces. Access to project is being checked already byuser_project
method.When I'll add API methods for canceling and restarting builds then I'll also add
:manage_builds
check to this methods. But for build listings and traces? Isn't project access level check not enough?
Added 1 commit:
- f39959d0 - Add some fixes to builds API
Added 1 commit:
- f4cff4dc - Modify build pagination to use 'paginate' helper
mentioned in issue #4264 (closed)
Added 1 commit:
- b2fbeb37 - Remove changes in 'paginate' helper
Added 1 commit:
- d2601211 - Add specs for build listings in API
Added 1 commit:
- 593d87ea - Add specs for build details/traces features in builds API
Added 1 commit:
- a17bf380 - Add cancel/retry features to builds API
Added 152 commits:
-
a17bf380...333d3d9e - 151 commits from branch
master
- 44dd4782 - Merge branch 'master' into ci/api-builds
-
a17bf380...333d3d9e - 151 commits from branch
@ayufan It's done. Please check if it have all features that You've wanted.
mentioned in issue #5599 (closed)
Added 1 commit:
- 628297fe - Remove incorrect 'default' values from method description
Added 118 commits:
-
628297fe...9b127028 - 117 commits from branch
master
- 962b97d8 - Merge branch 'master' into ci/api-builds
-
628297fe...9b127028 - 117 commits from branch
Added 1 commit:
- a862ade5 - Update ./doc/api/
- lib/api/builds.rb 0 → 100644
53 # Get a trace of a specific build of a project 54 # 55 # Parameters: 56 # id (required) - The ID of a project 57 # build_id (required) - The ID of a build 58 # Example Request: 59 # GET /projects/:id/build/:build_id/trace 60 get ':id/builds/:build_id/trace' do 61 build = get_build(params[:build_id]) 62 return not_found!(build) unless build 63 64 header 'Content-Disposition', "infile; filename=\"#{build.id}.log\"" 65 content_type 'text/plain' 66 env['api.format'] = :binary 67 68 trace = build.trace @ayufan I see one problem. When using
build.trace
we are sending trace withrunners_token
value replaced withxxxxxx
. If we will send file with workhorse 'just as it is', we will disclose the token.
After on-call discussion with @tmaczukin I left comments what can be improved. Overall looks good
.We should add to endpoint to download build artifacts as archive.
Edited by Kamil Trzcińśki- spec/requests/api/builds_spec.rb 0 → 100644
108 end 109 110 context 'unauthorized user' do 111 it 'should not cancel build' do 112 post api("/projects/#{project.id}/builds/#{build.id}/cancel") 113 114 expect(response.status).to eq(401) 115 end 116 end 117 end 118 119 describe 'GET /projects/:id/builds/:build_id/retry' do 120 context 'authorized user' do 121 context 'user with :manage_builds persmission' do 122 it 'should retry non-running build' do 123 post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user) Added 231 commits:
-
4e70f251...1ede18bf - 229 commits from branch
master
- a30377c6 - Merge branch 'master' into ci/api-builds
- 5dcfd7d0 - Add TODO notice to build trace feature in CI API
-
4e70f251...1ede18bf - 229 commits from branch
Added 1 commit:
- 5ba0232f - Fix :ci_build_with_trace factory
- doc/api/builds.md 0 → 100644
118 119 ``` 120 GET /projects/:id/repository/commits/:sha/builds 121 ``` 122 123 Parameters: 124 125 `id` (required) - The ID of a project 126 `sha` (required) - The SHA id of a commit 127 `scope` (optional) - The scope of builds to show (one or array of: pending, running, failed, success, canceled; if none provided showing all builds) 128 129 ```json 130 131 ``` 132 133 ## Get a single build mentioned in issue #6071 (closed)
When we are retrying a build, then a new record in
ci_builds
is created. And this new record references the same commit as an original build.Only first build for a commit is done automatically - each next is an effect of retry (@ayufan: correct me if I'm wrong!), so we could assume that a
created_at
field in every next record (with the same commit reference) is a time of retrying of previous one.Should we expose this information in build entity? I think not. Especially in listings - it would be quite expensive. And if someone want to find if build for a commit was retried then one can list all builds for a specific commit (by its SHA).
@ayufan what You think about this?
Added 1 commit:
- 470b3a48 - Fix doc/api/builds.md
@axil You can get all builds (including retried ones) or only latest. Exposing information how many time build was retried is time consuming so I would rather avoid implementing this.
/cc @tmaczukin
366 366 expose :id, :variables 367 367 end 368 369 class Runner < Grape::Entity 370 expose :id 371 expose :description 372 expose :active 373 expose :is_shared 374 expose :name 375 end 376 377 class Build < Grape::Entity 378 expose :id, :status, :stage, :name, :ref, :tag, :coverage 379 expose :created_at, :started_at, :finished_at 380 expose :user, with: UserFull 381 expose :download_url do |repo_obj, options| Added 1 commit:
- ab2c6cc0 - Add some fixes
LGTM
@DouweM @rspeicher Can you review?
Added 1 commit:
- 97338496 - Add some fixes after review
Added 1 commit:
- 990bd06c - Change :ci_build_canceled factory to :canceled trait
@rspeicher: I didn't deleted it, but I've pushed a commit which moved the code block a few lines lower. It seems like it's some bug - 'show/hide discussion' switch is showing the code box but without my comment inside.
And as for the
id
in the factory. When i remove theid 999
statement from factory definition then the test is failing. I'm getting:1) API::API GET /projects/:id/builds/:build_id(/trace)? authorized user should return specific build trace Failure/Error: expect(response.body).to eq(build_with_trace.trace) expected: nil got: "" (compared using ==) # ./spec/requests/api/builds_spec.rb:89:in `block (4 levels) in <top (required)>'
When I add the
id 999
statement in factory definition, then the test succeeds.I've tried to investigate why that happens and I've noticed this:
When
id
is present in factory, then - while creating build object from this factory - the trace filetmp/builds/2016_01/1/999.log
is created and is filled with trace value. This file is then read by API and bybuild_with_trace.trace
and both values are compared. And thanks to that, the test passes.But when
id
is removed from factory, then - while creating build object from this factory - the created trace file istmp/builds/2016_01/1/.log
. But a moment later API andbuild_with_trace.trace
readstmp/builds/2016_01/1/1.log
file! The second file does not exists. API is responding with empty content(""), andbuild_with_trace.trace
is returningnil
. And the test fails.Path to the trace file is created in
Ci::Build.path_to_trace
and it looks like:def path_to_trace "#{dir_to_trace}/#{id}.log" end
In real usage trace is submitted to existing build, which has
id
value. But here the factory is creating an object, setting values on it and then saving. While settingtrace
value, theid
is not present yet! Thats why the file name is.log
. But after saving theid
is filled with value from database and from this moment the file name is[some_integer].log
.As a conclusion:
Setting
id
in factory definition seems to be a simplest solution for that problem. If You have any other idea, I will hear it with pleasure :)Added 1 commit:
- 13032b71 - Add seperated 'describe' block for build trace specs
Added 121 commits:
-
13032b71...0e344aa2 - 118 commits from branch
master
- c9a16619 - Merge branch 'master' into ci/api-builds
- 92805119 - Update CHANGELOG [ci skip]
- 4cdbb2f6 - Modify builds API documentation style [ci skip]
Toggle commit list-
13032b71...0e344aa2 - 118 commits from branch
Setting id in factory definition seems to be a simplest solution for that problem. If You have any other idea, I will hear it with pleasure :)
Remove
id 999
and change theafter(:build)
toafter(:create)
, it will fix your problem./cc @rspeicher
Added 1 commit:
- 6a98fb03 - Remove hard-coded id from builds factory
Added 64 commits:
-
6a98fb03...f981da44 - 63 commits from branch
master
- 405b82af - Merge branch 'master' into ci/api-builds
-
6a98fb03...f981da44 - 63 commits from branch
Added 1 commit:
- 2c7d9cfa - Move Ci::Build#available_statuses to AVAILABLE_STATUSES constant in CommitStatus
Great! @DouweM will you do the honors?
@ayufan Gladly!
Enabled an automatic merge when the build for 2c7d9cfa succeeds
mentioned in commit ba42b033
mentioned in commit dd6c69e6
mentioned in issue #10852 (closed)
mentioned in issue #10421 (moved)
mentioned in issue #3001 (closed)