Skip to content
Snippets Groups Projects

Implement Commit Status API

Merged Kamil Trzcińśki requested to merge commit_status into master

This is preliminary implementation of Commit Status API, pretty much compatible with GitHub.

  1. The Commit Statuses are stored in separate table: ci_commit_status.
  2. The POST inserts a new row.
  3. To POST execute GitLab API post :id/repository/commits/:sha/status. This accepts dual authorization:
  • Using authorized user
  • Using ci-token to allow easy posting from CI Services
  1. This adds predefined variable to GitLab CI build environment: CI_BUILD_STATUS_URL, allowing to easy post status from within build (ex. with code coverage or other metrics).
  2. This adds statuses to commit's builds view.
  3. The commit's status is calculated taking into account status of all builds and all posted statuses.
  4. The commit statuses doesn't trigger notifications.
  5. The commit status API introduces two new privileges: read_commit_statuses and create_commit_status.
  6. We still miss a few tests and documentation updates for API and CI.

@dzaporozhets @sytses What do you think?

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
  • 43 end
    44
    45 def finished_at
    46 @created_at ||= created_at if complete?
    47 end
    48
    49 def duration
    50 if started? && complete?
    51 finished_at - started_at
    52 end
    53 end
    54
    55 def the_same
    56 CommitStatus.where(commit_id: commit_id, context: context, ref: ref)
    57 end
    58 end
    • @ayufan this class has too much common with Ci::Build. Are you still sure we want to keep same data in 2 different classes?

    • We can rename current Ci::Builds to CommitStatus and use it for both Status API and GitLab CI. The only difference so far is build trace which is ok. Maybe add type field to distinguish it.

    • Author Maintainer

      There's only small overlap in status part, but the rest of the logic is separate and Ci::Build does much more.

      I would rather see that separate.

    • Author Maintainer

      Ci::Build is used in many other contexts where CommitStatus doesn't fit: ex. runners.

      I can think that Ci::CommitStatus may be a base for Ci::Build. I kind of against renaming Ci::Build, because there's too much specific logic. I can also think of option where Ci::Build creates a new Ci::CommitStatus on status change.

      Secondly: each post currently creates separate CommitStatus entry, it doesn't update existing one for the same ref and context. I did that delibirately.

      Edited by Kamil Trzcińśki
    • Author Maintainer

      ^ Edited

    • @ayufan:

      I can also think of option where Ci::Build creates a new Ci::CommitStatus on status change.

      I think it makes sense to use commit statuses ourselves as well, just like we expect other CI systems to.

      each post currently creates separate CommitStatus entry, it doesn't update existing one for the same ref and context.

      That's what GitHub does as well right?

    • Author Maintainer

      I think it makes sense to use commit statuses ourselves as well, just like we expect other CI systems to.

      Yes, I thought that too.

      That's what GitHub does as well right?

      Yes, they keep track of all posted statuses. It makes sense, because this is something that comes from outside.

      CommitStatus is stateless object, that behaves more like an event.

  • @ayufan does this implement https://gitlab.com/gitlab-org/gitlab-ce/issues/2595 ?

    Can you comment there what the json payload is?

  • Author Maintainer

    @sytse Yes

    The :id/repository/commits/:sha/status accepts POST with:

          #   id (required) - The ID of a project
          #   sha (required) - The commit hash
          #   ref (optional) - The ref
          #   state (required) - The state of the status. Can be: pending, running, success, error or failure
          #   target_url (optional) - The target URL to associate with this status
          #   description (optional) - A short description of the status
          #   context (optional) - A string label to differentiate this status from the status of other systems. Default: "default"
    Edited by Kamil Trzcińśki
  • @ayufan Thanks! I asked for a comment there so I pasted it to the issue and propose we discuss there since there is more context there.

  • Author Maintainer

    Ci::Build is stateful, we have full event machine controlled by us. I didn't expect that we have the same for CommitStatus, where CommitStatus is pretty much informatory.

  • Author Maintainer

    These are model parameters for Ci::Build and Ci::CommitStatus:

    create_table "ci_builds", force: true do |t|
          t.integer  "commit_id"
          t.string   "ref"
          t.boolean  "tag"
          t.string   "stage"
          t.string   "status"
          t.datetime "finished_at"
          t.text     "trace"
          t.datetime "started_at"
          t.integer  "runner_id"
          t.float    "coverage"
          t.text     "commands"
          t.string   "name"
          t.boolean  "deploy",             default: false
          t.text     "options"
          t.boolean  "allow_failure",      default: false, null: false
          t.integer  "trigger_request_id"
          t.timestamps
    end
    
    create_table :ci_commit_statuses, force: true do |t|
          t.integer :commit_id
          t.string :ref
          t.string :status
          t.string :context, default: 'default'
          t.string :target_url
          t.string :description
          t.timestamps
    end

    There's very small overlap in actual data.

  • @ayufan to me it feels like the same thing, but I leave it up to @dzaporozhets and you.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 135 135
    136 136 def project_report_rules
    137 137 project_guest_rules + [
    138 :create_commit_status,
    139 :read_commit_statuses,
  • Kamil Trzcińśki Added 34 commits:

    Added 34 commits:

    • c7b90db4...1f11096c - 24 commits from branch master
    • d4bb91e0 - Fix API (+7 squashed commits)
    • f66736c8 - Expose Commit Status in RepoCommitDetail
    • 17d55a4f - Create CommitStatus specs
    • 6f4868f0 - Remove CI_BUILD_STATUS_URL
    • 434f18eb - Move skip_ci check to CreateCommitService
    • 263dc183 - Make Ci::Build to inherit from CommitStatus and add ExternalStatus
    • 8cbf143f - Fix me
    • 112de450 - WIP
    • 8f968ded - Fix tests
    • f70bf230 - Revert some changes
  • Author Maintainer

    @dzaporozhets I followed your guide:

    1. We have CommitStatus
    2. We have Ci::Build which inherits from CommitStatus
    3. I introduced ExternalStatus (which should be named ExternalCommitStatus) which inherits from CommitStatus and is created for all external POSTs

    Tests are broken, but more or less this is how it will finally look like.

  • @ayufan Having Ci::Build use CommitStatus wasn't an option?

    I'd rather have everything work around generic commit statuses, than have special cases for GitLab CI vs the rest.

  • @ayufan I think it looks much better now. Inheritance look good.

    Having Ci::Build use CommitStatus wasn't an option?

    @DouweM are you talking about removing ExternalCommitStatus and using CommitStatus instead?

  • Author Maintainer

    @dzaporozhets @DouweM is talking about leaving the Ci::Build and CommitStatus in separate tables as it was before, but instead of fuzzing status in Ci::Commit is to create a new CommitStatus on every Ci::Build status change, basically "posting" Ci::Build status to CommitStatus. This was also one of my previous propositions :)

  • The idea is that GitLab CI would function just like any other CI, posting commit statuses into the system. Would keep code cleaner because we wouldn't have any (or at least less) GitLab CI / other CI, Ci::Build / ExternalCommitStatus branching in logic or UI.

    Edited by Douwe Maan
  • I like Douwe's suggestion.
    We'd also be less likely to break something for other ci tools by accident while changing stuff here, because gitlab ci would break as well.

  • If @DouweM suggestion is straightforward to implement that would be nice.

  • Author Maintainer

    @DouweM

    After long thought I think that @dzaporozhets proposal makes sense, because CommitStatus and our current Build share a lot of logic. By having two separate objects we need to also have two separate implementations that we need to maintain. The problem that I see problem with posting pure commit status is that our CI exposes much more information that pure commit status can express. By going that direction we loose flexibility in the way how we can present and organise builds and our main goal should be to focus on making the GitLab CI nicest to use, but still having reasonable support for the others.

    We'd also be less likely to break something for other ci tools by accident while changing stuff here, because gitlab ci would break as well.

    We brake something we brake tests, and we also brake our CI integration. The external status is just pure data. All build related logic is in Ci::Build.

    Right now Ci::Commit is still overloaded with Ci::Build stuff, but some time in the future this will no longer be a case.

    @sytses It's somewhere in middle between the first and currently implemented approach. It's not hard to do it this way.

    Edited by Kamil Trzcińśki
  • @ayufan Thanks for commenting, I'll leave it up to you, @dzaporozhets and @DouweM to figure out.

  • @ayufan Makes sense to me!

  • Author Maintainer

    @dzaporozhets Please review.

  • Kamil Trzcińśki Title changed from [WIP] Implement Commit Status API to Implement Commit Status API

    Title changed from [WIP] Implement Commit Status API to Implement Commit Status API

  • Author Maintainer

    I'll finish API documentation part

  • Kamil Trzcińśki Added 83 commits:

    Added 83 commits:

    • f70bf230...5ffbf5fe - 82 commits from branch master
    • 914cfbd2 - Implement Commit Status API
  • Author Maintainer

    I'll move Ci::Build to Build separately.

  • Author Maintainer

    Documentation is updated.

  • Kamil Trzcińśki Added 3 commits:

    Added 3 commits:

    • 88749476 - Fix commit status POST URL
    • 7ef156a2 - Add author to statuses
    • 1808cc7e - Add Commit Status documentation
  • Author Maintainer

    @DouweM Can you review?

  • Added 1 commit:

    • 0aefeeb8 - Add Commit Status documentation
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 119 118 end
    120 119
    121 def builds_without_retry_for_ref(ref)
    122 builds.for_ref(ref).latest
    120 def builds_without_retry(ref = nil)
    121 if ref
    122 builds.for_ref(ref).latest
    123 else
    124 builds.latest
    125 end
    123 126 end
    124 127
    125 def retried_builds
    126 @retried_builds ||= (builds.order(id: :desc) - builds_without_retry)
    128 def retried
    129 @retried ||= (statuses.order(id: :desc) - statuses.latest)
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 20 20 extend Ci::Model
    21 21
    22 22 belongs_to :gl_project, class_name: '::Project', foreign_key: :gl_project_id
    23 has_many :builds, dependent: :destroy, class_name: 'Ci::Build'
    23 has_many :statuses, dependent: :destroy, class_name: 'CommitStatus'
    24 has_many :builds, class_name: 'Ci::Build'
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 44 after_transition any => [:success, :failed, :canceled] do |build, transition|
    45 build.update_attributes finished_at: Time.now
    46 end
    47
    48 state :pending, value: 'pending'
    49 state :running, value: 'running'
    50 state :failed, value: 'failed'
    51 state :success, value: 'success'
    52 state :canceled, value: 'canceled'
    53 end
    54
    55 delegate :sha, :short_sha, :gl_project,
    56 to: :commit, prefix: false
    57
    58 def before_sha
    59 Gitlab::Git::BLANK_SHA
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 11
    12 - if defined?(ref)
    13 %td
    14 = commit_status.ref
    15
    16 %td
    17 = commit_status.stage
    18
    19 %td
    20 = commit_status.description
    21 .pull-right
    22 - if commit_status.tags.any?
    23 - commit_status.tags.each do |tag|
    24 %span.label.label-primary
    25 = tag
    26 - if commit_status.try(:trigger_request)
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 13 %td
    14 = commit_status.ref
    15
    16 %td
    17 = commit_status.stage
    18
    19 %td
    20 = commit_status.description
    21 .pull-right
    22 - if commit_status.tags.any?
    23 - commit_status.tags.each do |tag|
    24 %span.label.label-primary
    25 = tag
    26 - if commit_status.try(:trigger_request)
    27 %span.label.label-info triggered
    28 - if commit_status.try(:allow_failure)
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 26 - if commit_status.try(:trigger_request)
    27 %span.label.label-info triggered
    28 - if commit_status.try(:allow_failure)
    29 %span.label.label-danger allowed to fail
    30
    31 %td.duration
    32 - if commit_status.duration
    33 #{duration_in_words(commit_status.finished_at, commit_status.started_at)}
    34
    35 %td.timestamp
    36 - if commit_status.finished_at
    37 %span #{time_ago_in_words commit_status.finished_at} ago
    38
    39 - if defined?(coverage)
    40 %td.coverage
    41 - if commit_status.try(:coverage)
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 39 # state (required) - The state of the status. Can be: pending, running, success, error or failure
    40 # target_url (optional) - The target URL to associate with this status
    41 # description (optional) - A short description of the status
    42 # name or context (optional) - A string label to differentiate this status from the status of other systems. Default: "default"
    43 # Examples:
    44 # POST /projects/:id/statuses/:sha
    45 post ':id/statuses/:sha' do
    46 required_attributes! [:state]
    47 attrs = attributes_for_keys [:ref, :target_url, :description, :context, :name]
    48 commit = @project.commit(params[:sha])
    49 not_found! 'Commit' unless commit
    50
    51 ci_commit = @project.ensure_ci_commit(commit.sha)
    52
    53 name = params[:name] || params[:context]
    54 status = GenericCommitStatus.running_or_pending.find_by(commit: ci_commit, name: name, ref: params[:ref])
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 228 229 expose :created_at
    229 230 end
    230 231
    232 class CommitStatus < Grape::Entity
    233 expose :id, :sha, :ref, :status, :name, :target_url, :description,
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 1 FactoryGirl.define do
    2 factory :commit_status, class: CommitStatus do
    3 started_at 'Di 29. Okt 09:51:28 CET 2013'
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 135 135
    136 136 def project_report_rules
    137 137 project_guest_rules + [
    138 :create_commit_status,
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 74 61
    75 62 def create_from(build)
  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 12 12 end
    13 13
    14 14 class Build < Grape::Entity
  • Author Maintainer

    @DouweM The all try's are for method that may do not yet exist in context of GenericCommitStatus.

  • Author Maintainer

    Are services that set statuses for skipped commits taken into account?

    Can you describe?

  • @ayufan Currently we are not creating a Ci::Commit when a commit contains [ci skip], so I wondered what would happend if an external service then sends a commit status for this commit even though we don't have a Ci::Commit. But I see that we have ensure_ci_commit which handles that.

  • Kamil Trzcińśki Added 5 commits:

    Added 5 commits:

  • Author Maintainer

    @DouweM I updated and simplified in a few places.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
  • 138 elsif pending?
    139 'pending'
    140 elsif running?
    141 'running'
    142 elsif canceled?
    143 'canceled'
    144 else
    145 'failed'
    131 end
    132
    133 @status ||= begin
    134 latest = latest_statuses
    135 latest.reject! { |status| status.try(&:allow_failure?) }
    136
    137 if latest.none?
    138 'skipped'
  • @ayufan 👍 Looks good to me

  • Added 1 commit:

  • Added 1 commit:

    • f40a6a48 - Fix broken commit.status test
  • Kamil Trzcińśki mentioned in merge request !1572 (merged)

    mentioned in merge request !1572 (merged)

  • Added 1 commit:

  • Added 1 commit:

  • Kamil Trzcińśki mentioned in merge request !1573 (merged)

    mentioned in merge request !1573 (merged)

  • Added 1 commit:

  • Added 1 commit:

  • Kamil Trzcińśki Status changed to merged

    Status changed to merged

  • mentioned in commit e3edd53a

  • mentioned in commit 45c0b74d

  • mentioned in commit a3a80eac

  • mentioned in issue #3080 (closed)

  • mentioned in issue #3081 (closed)

  • mentioned in issue #3082 (closed)

  • Please register or sign in to reply
    Loading