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.

    • 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.

    • 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
    • ^ 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?

    • 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.

  • 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.

  • 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
  • @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?

  • @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
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading