Skip to content
Snippets Groups Projects

Add builds API

Merged Tomasz Maczukin requested to merge ci/api-builds into master

References #4264 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • lib/api/builds.rb 0 → 100644
    1 module API
    2 # Projects builds API
    3 class Builds < Grape::API
    4 before { authenticate! }
    • Missing permission check

    • Author Maintainer

      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 by user_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?

    • Yes, we should add :manage_builds for retry, cancel.

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • f39959d0 - Add some fixes to builds API
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • f4cff4dc - Modify build pagination to use 'paginate' helper
  • Tomasz Maczukin Added 3 commits:

    Added 3 commits:

    • d398e78e - Add endpoint for getting builds for a specific commit
    • e7d0746d - Add 'not_found' notifications in build/trace details
    • 8d455503 - Add cancel/retry endpoints to build API
  • mentioned in issue #4264 (closed)

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • b2fbeb37 - Remove changes in 'paginate' helper
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • d2601211 - Add specs for build listings in API
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 593d87ea - Add specs for build details/traces features in builds API
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • a17bf380 - Add cancel/retry features to builds API
  • Tomasz Maczukin Added 152 commits:

    Added 152 commits:

  • Author Maintainer

    @ayufan It's done. Please check if it have all features that You've wanted.

  • mentioned in issue #5599 (closed)

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 628297fe - Remove incorrect 'default' values from method description
  • Tomasz Maczukin Added 118 commits:

    Added 118 commits:

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

  • 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
  • After on-call discussion with @tmaczukin I left comments what can be improved. Overall looks good :thumbsup:.

  • We should add to endpoint to download build artifacts as archive.

    Edited by Kamil Trzcińśki
  • 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)
  • Tomasz Maczukin Added 2 commits:

    Added 2 commits:

    • 549a2fa7 - Modify builds scope filtering in builds API
    • 1eb7b5ee - Modify entities for builds API
  • Tomasz Maczukin Added 2 commits:

    Added 2 commits:

    • d54bff2a - Change test access level from MASTER to DEVELOPER
    • 4eb27d7c - Add some modifications to builds API and specs
  • Tomasz Maczukin Added 2 commits:

    Added 2 commits:

    • 96bbc145 - Change commit builds URL in builds API
    • 4e70f251 - Update ./doc/api/builds.md
  • Tomasz Maczukin Added 231 commits:

    Added 231 commits:

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 049a7c32 - Fix :ci_build_with_trace factory
  • Tomasz Maczukin Added 1 commit:

    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
    • This seems duplicate of the one below (Get a single build of a project) that is lacking a heading. Can you fix it?

    • Author Maintainer

      The JSON example of builds list was inserted at invalid place. Thanks for noticing, I moved it to a proper line.

      Now the heading is also in proper place.

  • mentioned in issue #6071 (closed)

  • When retrying a build do we store in the db this action? It would be useful to expose this information, as well as the times the build got retried. I don't know if this is out of the scope of this MR, just throwing ideas :)

  • Author Maintainer

    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?

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

  • @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|
  • Ok guys, you know best :)

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

  • LGTM :thumbsup:

    @DouweM @rspeicher Can you review?

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 97338496 - Add some fixes after review
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 990bd06c - Change :ci_build_canceled factory to :canceled trait
  • Author Maintainer

    @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 the id 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 file tmp/builds/2016_01/1/999.log is created and is filled with trace value. This file is then read by API and by build_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 is tmp/builds/2016_01/1/.log. But a moment later API and build_with_trace.trace reads tmp/builds/2016_01/1/1.log file! The second file does not exists. API is responding with empty content(""), and build_with_trace.trace is returning nil. 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 setting trace value, the id is not present yet! Thats why the file name is .log. But after saving the id 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 :)

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 13032b71 - Add seperated 'describe' block for build trace specs
  • Tomasz Maczukin Added 121 commits:

    Added 121 commits:

  • Tomasz Maczukin Title changed from [WIP] Add builds API to Add builds API

    Title changed from [WIP] Add builds API to Add builds API

  • Tomasz Maczukin Title changed from Add builds API to [WIP] Add builds API

    Title changed from Add builds API to [WIP] Add builds API

  • @tmaczukin

    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 the after(:build) to after(:create), it will fix your problem.

    /cc @rspeicher

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 6a98fb03 - Remove hard-coded id from builds factory
  • Tomasz Maczukin Added 64 commits:

    Added 64 commits:

  • Tomasz Maczukin Title changed from [WIP] Add builds API to Add builds API

    Title changed from [WIP] Add builds API to Add builds API

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 2c7d9cfa - Move Ci::Build#available_statuses to AVAILABLE_STATUSES constant in CommitStatus
  • Author Maintainer

    @DouweM @ayufan: I've changed Ci::Build#available_statuses to CommitStatus::AVAILABLE_STATUSES constant.

  • Great! @DouweM will you do the honors? :thumbsup:

  • @ayufan Gladly!

  • Douwe Maan Enabled an automatic merge when the build for 2c7d9cfa succeeds

    Enabled an automatic merge when the build for 2c7d9cfa succeeds

  • Douwe Maan mentioned in commit ba42b033

    mentioned in commit ba42b033

  • Douwe Maan Status changed to merged

    Status changed to merged

  • Picked into 8-4-stable.

  • Douwe Maan mentioned in commit dd6c69e6

    mentioned in commit dd6c69e6

  • mentioned in issue #10852 (closed)

  • mentioned in issue #10421 (moved)

  • mentioned in issue #3001 (closed)

  • Please register or sign in to reply
    Loading