Commit f411f83d authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix/builds-api-nil-commit' into 'master'

Fix builds API response that did not include commit data

## What does this MR do?

This is fix for problem with builds API response not including information about commit this build is created for.

## What are the relevant issue numbers?

Closes #18476 

## 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)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [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)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4827
parents 408110f8 92984783
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 8.9.0 (unreleased) v 8.9.0 (unreleased)
- Fix builds API response not including commit data
- Fix error when CI job variables key specified but not defined - Fix error when CI job variables key specified but not defined
- Fix pipeline status when there are no builds in pipeline - Fix pipeline status when there are no builds in pipeline
- Fix Error 500 when using closes_issues API with an external issue tracker - Fix Error 500 when using closes_issues API with an external issue tracker
......
...@@ -54,6 +54,6 @@ class Projects::PipelinesController < Projects::ApplicationController ...@@ -54,6 +54,6 @@ class Projects::PipelinesController < Projects::ApplicationController
end end
def commit def commit
@commit ||= @pipeline.commit_data @commit ||= @pipeline.commit
end end
end end
...@@ -37,22 +37,22 @@ module Ci ...@@ -37,22 +37,22 @@ module Ci
end end
def git_author_name def git_author_name
commit_data.author_name if commit_data commit.try(:author_name)
end end
def git_author_email def git_author_email
commit_data.author_email if commit_data commit.try(:author_email)
end end
def git_commit_message def git_commit_message
commit_data.message if commit_data commit.try(:message)
end end
def short_sha def short_sha
Ci::Pipeline.truncate_sha(sha) Ci::Pipeline.truncate_sha(sha)
end end
def commit_data def commit
@commit ||= project.commit(sha) @commit ||= project.commit(sha)
rescue rescue
nil nil
......
...@@ -8,6 +8,8 @@ class CommitStatus < ActiveRecord::Base ...@@ -8,6 +8,8 @@ class CommitStatus < ActiveRecord::Base
belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :commit_id, touch: true belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :commit_id, touch: true
belongs_to :user belongs_to :user
delegate :commit, to: :pipeline
validates :pipeline, presence: true, unless: :importing? validates :pipeline, presence: true, unless: :importing?
validates_presence_of :name validates_presence_of :name
......
...@@ -24,8 +24,8 @@ ...@@ -24,8 +24,8 @@
%span.label.label-warning stuck %span.label.label-warning stuck
%p.commit-title %p.commit-title
- if commit_data = pipeline.commit_data - if commit = pipeline.commit
= link_to_gfm truncate(commit_data.title, length: 60), namespace_project_commit_path(@project.namespace, @project, commit_data.id), class: "commit-row-message" = link_to_gfm truncate(commit.title, length: 60), namespace_project_commit_path(@project.namespace, @project, commit.id), class: "commit-row-message"
- else - else
Cant find HEAD commit for this branch Cant find HEAD commit for this branch
......
...@@ -445,11 +445,7 @@ module API ...@@ -445,11 +445,7 @@ module API
expose :created_at, :started_at, :finished_at expose :created_at, :started_at, :finished_at
expose :user, with: User expose :user, with: User
expose :artifacts_file, using: BuildArtifactFile, if: -> (build, opts) { build.artifacts? } expose :artifacts_file, using: BuildArtifactFile, if: -> (build, opts) { build.artifacts? }
expose :commit, with: RepoCommit do |repo_obj, _options| expose :commit, with: RepoCommit
if repo_obj.respond_to?(:commit)
repo_obj.commit.commit_data
end
end
expose :runner, with: Runner expose :runner, with: Runner
end end
......
...@@ -2,7 +2,12 @@ require 'spec_helper' ...@@ -2,7 +2,12 @@ require 'spec_helper'
describe Ci::Build, models: true do describe Ci::Build, models: true do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:pipeline) { create(:ci_pipeline, project: project) }
let(:pipeline) do
create(:ci_pipeline, project: project,
sha: project.commit.id)
end
let(:build) { create(:ci_build, pipeline: pipeline) } let(:build) { create(:ci_build, pipeline: pipeline) }
it { is_expected.to validate_presence_of :ref } it { is_expected.to validate_presence_of :ref }
...@@ -658,4 +663,10 @@ describe Ci::Build, models: true do ...@@ -658,4 +663,10 @@ describe Ci::Build, models: true do
end end
end end
end end
describe '#commit' do
it 'returns commit pipeline has been created for' do
expect(build.commit).to eq project.commit
end
end
end end
require 'spec_helper' require 'spec_helper'
describe CommitStatus, models: true do describe CommitStatus, models: true do
let(:pipeline) { FactoryGirl.create :ci_pipeline } let(:project) { create(:project) }
let(:commit_status) { FactoryGirl.create :commit_status, pipeline: pipeline }
let(:pipeline) do
create(:ci_pipeline, project: project, sha: project.commit.id)
end
let(:commit_status) { create(:commit_status, pipeline: pipeline) }
it { is_expected.to belong_to(:pipeline) } it { is_expected.to belong_to(:pipeline) }
it { is_expected.to belong_to(:user) } it { is_expected.to belong_to(:user) }
...@@ -13,7 +18,7 @@ describe CommitStatus, models: true do ...@@ -13,7 +18,7 @@ describe CommitStatus, models: true do
it { is_expected.to delegate_method(:sha).to(:pipeline) } it { is_expected.to delegate_method(:sha).to(:pipeline) }
it { is_expected.to delegate_method(:short_sha).to(:pipeline) } it { is_expected.to delegate_method(:short_sha).to(:pipeline) }
it { is_expected.to respond_to :success? } it { is_expected.to respond_to :success? }
it { is_expected.to respond_to :failed? } it { is_expected.to respond_to :failed? }
it { is_expected.to respond_to :running? } it { is_expected.to respond_to :running? }
...@@ -116,7 +121,7 @@ describe CommitStatus, models: true do ...@@ -116,7 +121,7 @@ describe CommitStatus, models: true do
it { is_expected.to be > 0.0 } it { is_expected.to be > 0.0 }
end end
end end
describe :latest do describe :latest do
subject { CommitStatus.latest.order(:id) } subject { CommitStatus.latest.order(:id) }
...@@ -198,4 +203,10 @@ describe CommitStatus, models: true do ...@@ -198,4 +203,10 @@ describe CommitStatus, models: true do
end end
end end
end end
describe '#commit' do
it 'returns commit pipeline has been created for' do
expect(commit_status.commit).to eq project.commit
end
end
end end
...@@ -9,8 +9,8 @@ describe API::API, api: true do ...@@ -9,8 +9,8 @@ describe API::API, api: true do
let!(:project) { create(:project, creator_id: user.id) } let!(:project) { create(:project, creator_id: user.id) }
let!(:developer) { create(:project_member, :developer, user: user, project: project) } let!(:developer) { create(:project_member, :developer, user: user, project: project) }
let!(:reporter) { create(:project_member, :reporter, user: user2, project: project) } let!(:reporter) { create(:project_member, :reporter, user: user2, project: project) }
let(:pipeline) { create(:ci_pipeline, project: project)} let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id) }
let(:build) { create(:ci_build, pipeline: pipeline) } let!(:build) { create(:ci_build, pipeline: pipeline) }
describe 'GET /projects/:id/builds ' do describe 'GET /projects/:id/builds ' do
let(:query) { '' } let(:query) { '' }
...@@ -23,6 +23,11 @@ describe API::API, api: true do ...@@ -23,6 +23,11 @@ describe API::API, api: true do
expect(json_response).to be_an Array expect(json_response).to be_an Array
end end
it 'returns correct values' do
expect(json_response).not_to be_empty
expect(json_response.first['commit']['id']).to eq project.commit.id
end
context 'filter project with one scope element' do context 'filter project with one scope element' do
let(:query) { 'scope=pending' } let(:query) { 'scope=pending' }
...@@ -132,7 +137,7 @@ describe API::API, api: true do ...@@ -132,7 +137,7 @@ describe API::API, api: true do
describe 'GET /projects/:id/builds/:build_id/trace' do describe 'GET /projects/:id/builds/:build_id/trace' do
let(:build) { create(:ci_build, :trace, pipeline: pipeline) } let(:build) { create(:ci_build, :trace, pipeline: pipeline) }
before { get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user) } before { get api("/projects/#{project.id}/builds/#{build.id}/trace", api_user) }
context 'authorized user' do context 'authorized user' do
......
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