Skip to content
Snippets Groups Projects

Add runners API

Merged Tomasz Maczukin requested to merge ci/api-runners 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
  • @tmaczukin I made a comment, I'll review the rest tomorrow first thing.

  • 1 module API
    2 # Runners API
    3 class Runners < Grape::API
    4 before { authenticate! }
    5
    6 resource :runners do
    7 # Get available shared runners
    8 #
    9 # Example Request:
    10 # GET /runners
  • Maybe it would be good to return projects to which the runner is assigned?

  • Author Maintainer

    Maybe it would be good to return projects to which the runner is assigned?

    In the Get runner's details method? We can do, but I think that we should return only basic project data (like id, name, namespace), not entire project entity. What do You think?

  • Yes, basic data. And only project to which you have access, unless you are admin :)

  • Added 2 commits:

    • 00ad21db - Add note of deprecation in old Runners CI API
    • 2728b361 - Add Example response above each json output
  • Author Maintainer

    @axil: In 00ad21db You've added an deprecation note, but lib/ci/api/runners.rb is not deprecated! :) It's still used by runners to register/remove themselves from GitLab. Whole lib/ci/api will be left as an runners communication channel. Am I right, @ayufan?

    Edited by Tomasz Maczukin
  • Author Maintainer

    Yes, basic data. And only project to which you have access, unless you are admin :)

    if-s. if-s everywhere :smile:

    Will do :wink:

  • @tmaczukin Yes, you are right. We can say that at least for now it's used. It's definitely deprecated to be used by consumer API, since this is intended to be used ONLY by runners.

  • Seems I rushed! I'll change the note to what you guys said.

  • Tomasz Maczukin Added 4 commits:

    Added 4 commits:

    • 5216eef9 - Add some fixes in runners API documentation
    • 5e88f8b0 - Split /runners entrypoint to /runners and /runners/all
    • c5a94b7c - Fix runners API spec
    • cce5ae70 - Add associated project info to runner details output
  • Added 1 commit:

    • 36db1cad - Note that ci/api/runners.rb is still used
  • Added 2 commits:

    • c77c6ec7 - Correct PRIVATE-TOKEN to use dash instead of underscore
    • e06f5b3c - More fixes in runners doc
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 2c0e6121 - Add token to runner details output in API
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • fa668386 - Modify authentication check methods in runners API
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 20c44f63 - Limit projects to user available projects if user is not an admin
  • Added 1 commit:

    • b8ddfb14 - GET /runners is for specific runners only
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 0cbf4990 - Fix runners filtering
  • Tomasz Maczukin Added 569 commits:

    Added 569 commits:

    • 0cbf4990...22808a22 - 549 commits from branch master
    • 99417755 - Add basic runners management API
    • afc7da9a - Add feature to enable/disable runner in project
    • 71154113 - Add missing methods documentation; fix rubocop reported violation
    • a3c5211c - Fix runners filtering in API
    • 53b56c06 - Add runners API documentation [ci-skip]
    • 5f6ea6ef - Add note of deprecation in old Runners CI API
    • c8cba73d - Add Example response above each json output
    • 2b41b1ef - Add some fixes in runners API documentation
    • bd804649 - Split /runners entrypoint to /runners and /runners/all
    • ee494fed - Fix runners API spec
    • 9e01b8ee - Add associated project info to runner details output
    • 015e1bbd - Note that ci/api/runners.rb is still used
    • 7f0e0186 - Correct PRIVATE-TOKEN to use dash instead of underscore
    • eea956df - More fixes in runners doc
    • bb60fa43 - Add token to runner details output in API
    • a0172414 - Modify authentication check methods in runners API
    • 434fa1d9 - Limit projects to user available projects if user is not an admin
    • 7167bc59 - GET /runners is for specific runners only
    • ed423406 - Fix runners filtering
    • 37644145 - Modify runner projects selecting method in runners API
  • 377 383 expose :name
    378 384 end
    379 385
    386 class RunnerDetails < Runner
    387 expose :tag_list
    388 expose :version, :revision, :platform, :architecture
    389 expose :contacted_at, as: :last_contact
    390 expose :token, if: lambda { |runner, options| options[:current_user].is_admin? || !runner.is_shared? }
    391 expose :projects, with: Entities::RunnerProjectDetails do |runner, options|
    392 if options[:current_user].is_admin?
    393 runner.projects
    394 else
    395 options[:current_user].authorized_projects.where(id: runner.projects)
    • Author Maintainer

      @ayufan I did this like You suggested. It still generates an union of three selects:

      query

      This query looks like a monster! :smile: But at least the code is now more clean.

      This is showing all projects associated with the runner and for which the user has access. I still don't see the purpose to show the user only projects where he has master role.

    • Nice analysis we should talk maybe we can optimise that somehow :dagger:

    • Author Maintainer

      On the other hand - most of steps are using index or doing hash joins. It should not be hard for PostgreSQL. I'm only worried about those sequence scans on projects and members tables. They can be big and sequence scan can consume much IO resources. But... 75% of this query (from Hash Agregate to bottom) is used right now in many places - this is the User::authorized_projects query. So maybe I'm worrying unnecessarily.

  • Tomasz Maczukin Milestone changed to 8.5

    Milestone changed to 8.5

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 3c75b013 - Modify and fix output of delete and update of a runner
  • Author Maintainer

    @ayufan: I think, that this is done. There is only a question about this project list in RunnerDetails, but the rest is finished.

    Lets review it, decide what to do with RunnerDetails and merge it :)

  • @tmaczukin Ok, lets follow up on this tomorrow morning :)

  • 239 {
    240 "active": true,
    241 "description": "test-2-20150125",
    242 "id": 8,
    243 "is_shared": false,
    244 "name": null
    245 },
    246 {
    247 "active": true,
    248 "description": "development_runner",
    249 "id": 5,
    250 "is_shared": true,
    251 "name": null
    252 }
    253 ]
    254 ```
    • I think it would be great to have a list of runner's tags here.

    • Author Maintainer

      We can add tags list here, but that means one additional SELECT for each runner on list. For default 20 entities per page that means 21 DB queries where 20 of them are SELECTs for tags.

  • 120 end
    121
    122 helpers do
    123 def filter_runners(runners, scope, options = {})
    124 return runners unless scope.present?
    125
    126 available_scopes = ::Ci::Runner::AVAILABLE_SCOPES
    127 if options[:without]
    128 available_scopes = available_scopes - options[:without]
    129 end
    130
    131 if (available_scopes & [scope]).empty?
    132 render_api_error!('Scope contains invalid value', 400)
    133 end
    134
    135 runners.send(scope)
    • Are you sure that this is 100% safe?

    • Author Maintainer

      scope is validated against limited list, so it shouldn't have invalid value. We can use if.. elsif or case constructions here, but this should work just like the above. And this is more DRY-like. If we will want to create new or remove some scopes, we should only modify the model (and AVAILABLE_SCOPES const in it). filter_runners method would still work in that case.

  • 8 let(:user2) { create(:user) }
    9 let!(:project) { create(:project, creator_id: user.id) }
    10 let!(:project2) { create(:project, creator_id: user.id) }
    11 let!(:master) { create(:project_member, user: user, project: project, access_level: ProjectMember::MASTER) }
    12 let!(:master2) { create(:project_member, user: user, project: project2, access_level: ProjectMember::MASTER) }
    13 let!(:developer) { create(:project_member, user: user2, project: project, access_level: ProjectMember::REPORTER) }
    14 let!(:shared_runner) { create(:ci_shared_runner, tag_list: ['mysql', 'ruby'], active: true) }
    15 let!(:specific_runner) { create(:ci_specific_runner, tag_list: ['mysql', 'ruby']) }
    16 let!(:specific_runner_project) { create(:ci_runner_project, runner: specific_runner, project: project) }
    17 let!(:specific_runner2) { create(:ci_specific_runner) }
    18 let!(:specific_runner2_project) { create(:ci_runner_project, runner: specific_runner2, project: project2) }
    19 let!(:unused_specific_runner) { create(:ci_specific_runner) }
    20 let!(:two_projects_runner) { create(:ci_specific_runner) }
    21 let!(:two_projects_runner_project) { create(:ci_runner_project, runner: two_projects_runner, project: project) }
    22 let!(:two_projects_runner_project2) { create(:ci_runner_project, runner: two_projects_runner, project: project2) }
    23
    • We should put lets inside a scope that those methods are needed in, unless it is needed everywhere.

    • Author Maintainer

      Only :specific_runner2 is used in one describe block. I will move it to this block. Rest of variables is reused across different describe blocks, so they must be defined globally.

  • 100 end
    101 end
    102 end
    103
    104 describe 'GET /runners/:id' do
    105 context 'admin user' do
    106 it "should return runner's details" do
    107 get api("/runners/#{specific_runner.id}", admin)
    108
    109 expect(response.status).to eq(200)
    110 expect(json_response['description']).to eq(specific_runner.description)
    111 end
    112
    113 it "should return shared runner's details" do
    114 get api("/runners/#{shared_runner.id}", admin)
    115
    • Whether it is a specific runner or a shared runner it is a context, so in each relevant example this should go into context "specific runner" or context "shared runner"

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • e18a60eb - Add some modifications to spec/requests/api/runners_spec.rb
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 799e62bf - Move :runner_id param to POST body when enabling specific runner in project
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • dbd69ab4 - Update docummentation - specific runner enabling
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 52b961ff - iChange name and path to name_with_namespace and path_with_namespace in …
  • I see it green. Is it ready @tmaczukin? :)

  • Author Maintainer

    @ayufan: There is opened discussion about runner projects query (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2640/diffs#cc752c66542cbeeb21ca7a57c430836b865580c9_380_395) and scope filtering with runner.send(scope) (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2640/diffs#94e215001dc1fb1699bdc278b0ff146bacc25bcd_0_135). If we decide to leave it as it is, then yes, in my opinion it is ready :)

  • Kamil Trzcińśki Title changed from [WIP] Add runners API to Add runners API

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

  • @DouweM @rspeicher Could you guys review?

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • f8b7723f - Change interpolation to named placeholder in owned_or_shared scope
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 187fc7e3 - Reorganize let statements in spec/requests/api/runners_spec.rb
  • Tomasz Maczukin Added 2 commits:

    Added 2 commits:

    • e89b1f3c - Reorganize let statements in spec/requests/api/runners_spec.rb
    • c6422806 - Change .map{...}.inject{...} to any?{...} for searching shared runners
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • a309df36 - Modify expectations for update runner feature
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 2af68720 - Remove unnecessary parameters
  • Author Maintainer

    @rspeicher: I've implemented and/or replied on Your comments. You can check the code again.

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • c90986ab - Replace Entities::RunnerProjectDetails with Entities::ForkedFromProject
  • Tomasz Maczukin Added 2 commits:

    Added 2 commits:

    • 45ca2396 - Change last_contact to contacted_at
    • c02b1602 - Refactorize ci_runner factory and let definitions in runners API spec
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • 22716cf4 - Fix old usages of ci_runner factory
  • Author Maintainer

    Further fixes are implemented and all is green.

  • @DouweM Could you look?

  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • aeb5b0fb - Add CHANGELOG entry
  • Tomasz Maczukin Added 1 commit:

    Added 1 commit:

    • e98b6335 - Rename Entities::ForkedFromProject to Entities::BasicProjectDetails
  • Tomasz Maczukin Added 276 commits:

    Added 276 commits:

    • e98b6335...e586858e - 240 commits from branch master
    • 128be3c0 - Add basic runners management API
    • d42ced44 - Add feature to enable/disable runner in project
    • 8c37f0ff - Add missing methods documentation; fix rubocop reported violation
    • 53f775ae - Fix runners filtering in API
    • 43c8daf3 - Add runners API documentation [ci-skip]
    • 1532d28c - Add note of deprecation in old Runners CI API
    • cd62c747 - Add Example response above each json output
    • b56ee397 - Add some fixes in runners API documentation
    • 81ced6f5 - Split /runners entrypoint to /runners and /runners/all
    • 16b3368a - Fix runners API spec
    • f562a477 - Add associated project info to runner details output
    • d4f1bcdd - Note that ci/api/runners.rb is still used
    • a09ae173 - Correct PRIVATE-TOKEN to use dash instead of underscore
    • dc32af95 - More fixes in runners doc
    • 553bac57 - Add token to runner details output in API
    • b58744cd - Modify authentication check methods in runners API
    • f21b15d5 - Limit projects to user available projects if user is not an admin
    • 97c88966 - GET /runners is for specific runners only
    • 36e7ffea - Fix runners filtering
    • 24eed1c5 - Modify runner projects selecting method in runners API
    • d1ac00ae - Modify and fix output of delete and update of a runner
    • dc182dc5 - Add some modifications to spec/requests/api/runners_spec.rb
    • b36116f9 - Move :runner_id param to POST body when enabling specific runner in project
    • 05e73356 - Update docummentation - specific runner enabling
    • 4ebadb77 - iChange name and path to name_with_namespace and path_with_namespace in …
    • 2ef196de - Change interpolation to named placeholder in owned_or_shared scope
    • 957c4de9 - Reorganize let statements in spec/requests/api/runners_spec.rb
    • d38322b8 - Change .map{...}.inject{...} to any?{...} for searching shared runners
    • a5540b38 - Modify expectations for update runner feature
    • f8f492e5 - Remove unnecessary parameters
    • 7ea60c85 - Replace Entities::RunnerProjectDetails with Entities::ForkedFromProject
    • e4d2f997 - Change last_contact to contacted_at
    • acfe25ed - Refactorize ci_runner factory and let definitions in runners API spec
    • 49d5c35b - Fix old usages of ci_runner factory
    • 98236881 - Add CHANGELOG entry
    • f2f6f773 - Rename Entities::ForkedFromProject to Entities::BasicProjectDetails
  • mentioned in commit 1817b766

  • Kamil Trzcińśki Status changed to merged

    Status changed to merged

  • Kamil Trzcińśki Added ~149423 label

    Added ~149423 label

  • @DouweM I merged the Runners API :)

  • mentioned in commit 778da502

  • Picked into 8-5-stable.

  • username-removed-128633 Removed ~149423 label

    Removed ~149423 label

  • Please register or sign in to reply
    Loading