Implement Commit Status API
This is preliminary implementation of Commit Status API, pretty much compatible with GitHub.
- The Commit Statuses are stored in separate table: ci_commit_status.
- The POST inserts a new row.
- 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
- 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).
- This adds statuses to commit's builds view.
- The commit's status is calculated taking into account status of all builds and all posted statuses.
- The commit statuses doesn't trigger notifications.
- The commit status API introduces two new privileges:
read_commit_statuses
andcreate_commit_status
. - We still miss a few tests and documentation updates for API and CI.
@dzaporozhets @sytses What do you think?
Merge request reports
Activity
Added 1 commit:
- c7b90db4 - Fix API
- Edited by Kamil Trzcińśki
- app/models/commit_status.rb 0 → 100644
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?
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ńśkiI 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?
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?
@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.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.
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
Toggle commit list-
c7b90db4...1f11096c - 24 commits from branch
@dzaporozhets I followed your guide:
- We have CommitStatus
- We have Ci::Build which inherits from CommitStatus
- 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 Awesome!
@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.
@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 MaanIf @DouweM suggestion is straightforward to implement that would be nice.
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!
@ayufan
👍 @dzaporozhets Please review.
@DouweM Can you review?
Added 1 commit:
- 0aefeeb8 - Add Commit Status documentation
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) 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' - app/models/commit_status.rb 0 → 100644
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 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) 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) 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) - lib/api/commit_statuses.rb 0 → 100644
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]) 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, - spec/factories/commit_statuses.rb 0 → 100644
@DouweM The all try's are for method that may do not yet exist in context of GenericCommitStatus.
@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 aCi::Commit
. But I see that we haveensure_ci_commit
which handles that.@DouweM I updated and simplified in a few places.
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 meAdded 1 commit:
- d266bfc3 - Fix broken tests
Added 1 commit:
- f40a6a48 - Fix broken commit.status test
mentioned in merge request !1572 (merged)
Added 1 commit:
- daca1c65 - Fix broken tests
Added 1 commit:
- 766da5fa - Fix broken matrix_for_ref?
mentioned in merge request !1573 (merged)
Added 1 commit:
- 4bf69b0b - Fix feature tests
Added 1 commit:
- e7cc554c - Fix retry and cancel URLs
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)