Merge branch 'ci-commit-status-skipped' into 'master'

Don't create CI status for refs that doesn't have .gitlab-ci.yml, even if the builds are enabled

Fixes #3827 
Fixes #4157

/cc @grzesiek @dblessing 

See merge request !2139

CHANGELOG

@@ -26,6 +26,7 @@ v 8.3.0 (unreleased)
   - Migrate all CI::Services and CI::WebHooks to Services and WebHooks
   - Don't show project fork event as "imported"
   - Add API endpoint to fetch merge request commits list
+  - Don't create CI status for refs that doesn't have .gitlab-ci.yml, even if the builds are enabled
   - Expose events API with comment information and author info
   - Fix: Ensure "Remove Source Branch" button is not shown when branch is being deleted. #3583
   - Run custom Git hooks when branch is created or deleted.

app/models/ci/commit.rb

@@ -218,16 +218,6 @@ module Ci
       update!(committed_at: DateTime.now)
     end
 
-    ##
-    # This method checks if build status should be displayed.
-    #
-    # Build status should be available only if builds are enabled
-    # on project level and `.gitlab-ci.yml` file is present.
-    #
-    def show_build_status?
-      project.builds_enabled? && ci_yaml_file
-    end
-
     private
 
     def save_yaml_error(error)

app/services/create_commit_builds_service.rb

@@ -16,9 +16,23 @@ class CreateCommitBuildsService
       return false
     end
 
-    tag = Gitlab::Git.tag_ref?(origin_ref)
-    commit = project.ensure_ci_commit(sha)
+    commit = project.ci_commit(sha)
+    unless commit
+      commit = project.ci_commits.new(sha: sha)
+
+      # Skip creating ci_commit when no gitlab-ci.yml is found
+      unless commit.ci_yaml_file
+        return false
+      end
+
+      # Create a new ci_commit
+      commit.save!
+    end
+
+    # Skip creating builds for commits that have [ci skip]
     unless commit.skip_ci?
+      # Create builds for commit
+      tag = Gitlab::Git.tag_ref?(origin_ref)
       commit.update_committed!
       commit.create_builds(ref, tag, user)
     end

app/views/projects/commit/_commit_box.html.haml

@@ -40,7 +40,7 @@
   - @commit.parents.each do |parent|
     = link_to parent.short_id, namespace_project_commit_path(@project.namespace, @project, parent), class: "monospace"
 
-- if @ci_commit && @ci_commit.show_build_status?
+- if @ci_commit
   .pull-right
     = link_to ci_status_path(@ci_commit), class: "ci-status ci-#{@ci_commit.status}" do
       = ci_status_icon(@ci_commit)

app/views/projects/commits/_commit.html.haml

@@ -17,7 +17,7 @@
           %a.text-expander.js-toggle-button ...
 
       .pull-right
-        - if ci_commit && ci_commit.show_build_status?
+        - if ci_commit
           = render_ci_status(ci_commit)
            
         = clipboard_button(clipboard_text: commit.id)

spec/features/commits_spec.rb

@@ -19,30 +19,13 @@ describe 'Commits' do
     let!(:build) { FactoryGirl.create :ci_build, commit: commit }
 
     describe 'Project commits' do
-      context 'builds enabled' do
-        context '.gitlab-ci.yml found' do
-          before do
-            visit namespace_project_commits_path(project.namespace, project, :master)
-          end
-
-          it 'should show build status' do
-            page.within("//li[@id='commit-#{commit.short_sha}']") do
-              expect(page).to have_css(".ci-status-link")
-            end
-          end
-        end
+      before do
+        visit namespace_project_commits_path(project.namespace, project, :master)
+      end
 
-        context 'no .gitlab-ci.yml found' do
-          before do
-            stub_ci_commit_yaml_file(nil)
-            visit namespace_project_commits_path(project.namespace, project, :master)
-          end
-
-          it 'should not show build status' do
-            page.within("//li[@id='commit-#{commit.short_sha}']") do
-              expect(page).to have_no_css(".ci-status-link")
-            end
-          end
+      it 'should show build status' do
+        page.within("//li[@id='commit-#{commit.short_sha}']") do
+          expect(page).to have_css(".ci-status-link")
         end
       end
     end

spec/services/create_commit_builds_service_spec.rb

@@ -52,7 +52,7 @@ describe CreateCommitBuildsService, services: true do
       end
     end
 
-    it 'skips commits without .gitlab-ci.yml' do
+    it 'skips creating ci_commit for refs without .gitlab-ci.yml' do
       stub_ci_commit_yaml_file(nil)
       result = service.execute(project, user,
                                ref: 'refs/heads/0_1',
@@ -60,13 +60,11 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: [{ message: 'Message' }]
                               )
-      expect(result).to be_persisted
-      expect(result.builds.any?).to be_falsey
-      expect(result.status).to eq('skipped')
-      expect(result.yaml_errors).to be_nil
+      expect(result).to be_falsey
+      expect(Ci::Commit.count).to eq(0)
     end
 
-    it 'skips commits if yaml is invalid' do
+    it 'fails commits if yaml is invalid' do
       message = 'message'
       allow_any_instance_of(Ci::Commit).to receive(:git_commit_message) { message }
       stub_ci_commit_yaml_file('invalid: file: file')
@@ -77,6 +75,7 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: commits
                               )
+      expect(commit).to be_persisted
       expect(commit.builds.any?).to be false
       expect(commit.status).to eq('failed')
       expect(commit.yaml_errors).to_not be_nil
@@ -97,6 +96,7 @@ describe CreateCommitBuildsService, services: true do
                                  after: '31das312',
                                  commits: commits
                                 )
+        expect(commit).to be_persisted
         expect(commit.builds.any?).to be false
         expect(commit.status).to eq("skipped")
       end
@@ -112,6 +112,7 @@ describe CreateCommitBuildsService, services: true do
                                  commits: commits
                                 )
 
+        expect(commit).to be_persisted
         expect(commit.builds.first.name).to eq("staging")
       end
 
@@ -124,6 +125,7 @@ describe CreateCommitBuildsService, services: true do
                                  after: '31das312',
                                  commits: commits
                                 )
+        expect(commit).to be_persisted
         expect(commit.builds.any?).to be false
         expect(commit.status).to eq("skipped")
         expect(commit.yaml_errors).to be_nil
@@ -140,6 +142,7 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: commits
                               )
+      expect(commit).to be_persisted
       expect(commit.builds.count(:all)).to eq(2)
 
       commit = service.execute(project, user,
@@ -148,6 +151,7 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: commits
                               )
+      expect(commit).to be_persisted
       expect(commit.builds.count(:all)).to eq(2)
     end
 
@@ -163,6 +167,7 @@ describe CreateCommitBuildsService, services: true do
                                commits: commits
                               )
 
+      expect(commit).to be_persisted
       expect(commit.status).to eq("failed")
       expect(commit.builds.any?).to be false
     end