Commit 5fdd92df authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'feature/builds-register-change-404-to-204' into 'master'

Change response for /ci/api/v1/builds/register.json from 404 to 204

## What does this MR do?

To check if there are new builds scheduled for a runner, runner is sending `POST /ci/api/v1/builds/register.json` requests. If there is a build then a `200` response with build data is returned. However if there is no builds scheduled for this runner it receives a `404 Not Found` response. This may end with a lot of `404 Not Found` lines in logs that are an expected behavior (please read #14445 for a reference).

Since `v1.0.1` version GitLab Runner is ready to receive a `204 No Content` response in case of no builds.

This MR adds a support for this status code for each Runner's version that can be determined while handling `POST /ci/api/v1/builds/register.json` request.

## Are there points in the code the reviewer needs to double check?

Specs in `spec/requests/ci/api/builds_spec.rb`

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [ ] ~~Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)~~
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Fixes #14445

See merge request !6225
parents 82b8cc5d e940d97d
......@@ -130,6 +130,7 @@ v 8.12.0 (unreleased)
- Add notification_settings API calls !5632 (mahcsig)
- Remove duplication between project builds and admin builds view !5680 (Katarzyna Kobierska Ula Budziszewska)
- Deleting source project with existing fork link will close all related merge requests !6177 (Katarzyna Kobierska Ula Budziszeska)
- Return 204 instead of 404 for /ci/api/v1/builds/register.json if no builds are scheduled for a runner !6225
v 8.11.6 (unreleased)
- Fix an error where we were unable to create a CommitStatus for running state
......
......@@ -38,6 +38,15 @@ POST /ci/api/v1/builds/register
curl --request POST "https://gitlab.example.com/ci/api/v1/builds/register" --form "token=t0k3n"
```
**Responses:**
| Status | Data |Description |
|--------|------|---------------------------------------------------------------------------|
| `201` | yes | When a build is scheduled for a runner |
| `204` | no | When no builds are scheduled for a runner (for GitLab Runner >= `v1.3.0`) |
| `403` | no | When invalid token is used or no token is sent |
| `404` | no | When no builds are scheduled for a runner (for GitLab Runner < `v1.3.0`) **or** when the runner is set to `paused` in GitLab runner's configuration page |
### Update details of an existing build
```
......
......@@ -269,6 +269,10 @@ module API
render_api_error!('304 Not Modified', 304)
end
def no_content!
render_api_error!('204 No Content', 204)
end
def render_validation_error!(model)
if model.errors.any?
render_api_error!(model.errors.messages || '400 Bad Request', 400)
......
......@@ -27,7 +27,7 @@ module Ci
else
Gitlab::Metrics.add_event(:build_not_found)
not_found!
build_not_found!
end
end
......
......@@ -32,6 +32,14 @@ module Ci
end
end
def build_not_found!
if headers['User-Agent'].match(/gitlab-ci-multi-runner \d+\.\d+\.\d+(~beta\.\d+\.g[0-9a-f]+)? /)
no_content!
else
not_found!
end
end
def current_runner
@runner ||= Runner.find_by_token(params[:token].to_s)
end
......
......@@ -30,5 +30,9 @@ FactoryGirl.define do
trait :shared do
is_shared true
end
trait :inactive do
active false
end
end
end
......@@ -15,6 +15,25 @@ describe Ci::API::API do
describe "POST /builds/register" do
let!(:build) { create(:ci_build, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
let(:user_agent) { 'gitlab-ci-multi-runner 1.5.2 (1-5-stable; go1.6.3; linux/amd64)' }
shared_examples 'no builds available' do
context 'when runner sends version in User-Agent' do
context 'for stable version' do
it { expect(response).to have_http_status(204) }
end
context 'for beta version' do
let(:user_agent) { 'gitlab-ci-multi-runner 1.6.0~beta.167.g2b2bacc (1-5-stable; go1.6.3; linux/amd64)' }
it { expect(response).to have_http_status(204) }
end
end
context "when runner doesn't send version in User-Agent" do
let(:user_agent) { 'Go-http-client/1.1' }
it { expect(response).to have_http_status(404) }
end
end
it "starts a build" do
register_builds info: { platform: :darwin }
......@@ -33,36 +52,30 @@ describe Ci::API::API do
context 'when builds are finished' do
before do
build.success
end
it "returns 404 error if no builds for specific runner" do
register_builds
expect(response).to have_http_status(404)
end
it_behaves_like 'no builds available'
end
context 'for other project with builds' do
before do
build.success
create(:ci_build, :pending)
end
it "returns 404 error if no builds for shared runner" do
register_builds
expect(response).to have_http_status(404)
end
it_behaves_like 'no builds available'
end
context 'for shared runner' do
let(:shared_runner) { create(:ci_runner, token: "SharedRunner") }
it "should return 404 error if no builds for shared runner" do
before do
register_builds shared_runner.token
expect(response).to have_http_status(404)
end
it_behaves_like 'no builds available'
end
context 'for triggered build' do
......@@ -136,18 +149,27 @@ describe Ci::API::API do
end
context 'when runner is not allowed to pick untagged builds' do
before { runner.update_column(:run_untagged, false) }
it 'does not pick build' do
before do
runner.update_column(:run_untagged, false)
register_builds
end
expect(response).to have_http_status 404
it_behaves_like 'no builds available'
end
end
context 'when runner is paused' do
let(:inactive_runner) { create(:ci_runner, :inactive, token: "InactiveRunner") }
before do
register_builds inactive_runner.token
end
it { expect(response).to have_http_status 404 }
end
def register_builds(token = runner.token, **params)
post ci_api("/builds/register"), params.merge(token: token)
post ci_api("/builds/register"), params.merge(token: token), { 'User-Agent' => user_agent }
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment