Merge branch 'ci-yaml-validation' into 'master'

Commits without .gitlab-ci.yml are marked as skipped

- Commits without .gitlab-ci.yml are marked as skipped
- Save detailed error when YAML syntax

This also fixes: #3521 #3546 

/cc @jacobvosmaer 


See merge request !1827

CHANGELOG

@@ -16,6 +16,7 @@ v 8.2.0
   - Add allow_failure field to commit status API (Stan Hu)
   - Commits without .gitlab-ci.yml are marked as skipped
   - Save detailed error when YAML syntax is invalid
+  - Since GitLab CI is enabled by default, remove enabling it by pushing .gitlab-ci.yml
   - Added build artifacts
   - Improved performance of replacing references in comments
   - Show last project commit to default branch on project home page

app/controllers/ci/lints_controller.rb

@@ -15,10 +15,10 @@ module Ci
         @builds = @config_processor.builds
         @status = true
       end
-    rescue Ci::GitlabCiYamlProcessor::ValidationError => e
+    rescue Ci::GitlabCiYamlProcessor::ValidationError, Psych::SyntaxError => e
       @error = e.message
       @status = false
-    rescue Exception
+    rescue
       @error = "Undefined error"
       @status = false
     end

app/models/ci/commit.rb

@@ -188,13 +188,13 @@ module Ci
     end
 
     def config_processor
+      return nil unless ci_yaml_file
       @config_processor ||= Ci::GitlabCiYamlProcessor.new(ci_yaml_file, gl_project.path_with_namespace)
-    rescue Ci::GitlabCiYamlProcessor::ValidationError => e
+    rescue Ci::GitlabCiYamlProcessor::ValidationError, Psych::SyntaxError => e
       save_yaml_error(e.message)
       nil
-    rescue Exception => e
-      logger.error e.message + "\n" + e.backtrace.join("\n")
-      save_yaml_error("Undefined yaml error")
+    rescue
+      save_yaml_error("Undefined error")
       nil
     end
 

app/services/git_push_service.rb

@@ -58,12 +58,6 @@ class GitPushService
 
     @push_data = build_push_data(oldrev, newrev, ref)
 
-    # If CI was disabled but .gitlab-ci.yml file was pushed
-    # we enable CI automatically
-    if !project.builds_enabled? && gitlab_ci_yaml?(newrev)
-      project.enable_ci
-    end
-
     EventCreateService.new.push(project, user, @push_data)
     project.execute_hooks(@push_data.dup, :push_hooks)
     project.execute_services(@push_data.dup, :push_hooks)
@@ -134,10 +128,4 @@ class GitPushService
   def commit_user(commit)
     commit.author || user
   end
-
-  def gitlab_ci_yaml?(sha)
-    @project.repository.blob_at(sha, '.gitlab-ci.yml')
-  rescue Rugged::ReferenceError
-    nil
-  end
 end

spec/lib/ci/gitlab_ci_yaml_processor_spec.rb

@@ -425,8 +425,12 @@ module Ci
     end
 
     describe "Error handling" do
+      it "fails to parse YAML" do
+        expect{GitlabCiYamlProcessor.new("invalid: yaml: test")}.to raise_error(Psych::SyntaxError)
+      end
+
       it "indicates that object is invalid" do
-        expect{GitlabCiYamlProcessor.new("invalid_yaml\n!ccdvlf%612334@@@@")}.to raise_error(GitlabCiYamlProcessor::ValidationError)
+        expect{GitlabCiYamlProcessor.new("invalid_yaml")}.to raise_error(GitlabCiYamlProcessor::ValidationError)
       end
 
       it "returns errors if tags parameter is invalid" do

spec/services/ci/create_commit_service_spec.rb

@@ -53,7 +53,7 @@ module Ci
         end
       end
 
-      it 'fails commits without .gitlab-ci.yml' do
+      it 'skips commits without .gitlab-ci.yml' do
         stub_ci_commit_yaml_file(nil)
         result = service.execute(project, user,
                                  ref: 'refs/heads/0_1',
@@ -63,7 +63,24 @@ module Ci
         )
         expect(result).to be_persisted
         expect(result.builds.any?).to be_falsey
-        expect(result.status).to eq('failed')
+        expect(result.status).to eq('skipped')
+        expect(result.yaml_errors).to be_nil
+      end
+
+      it 'skips 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')
+        commits = [{ message: message }]
+        commit = service.execute(project, user,
+                                 ref: 'refs/tags/0_1',
+                                 before: '00000000',
+                                 after: '31das312',
+                                 commits: commits
+        )
+        expect(commit.builds.any?).to be false
+        expect(commit.status).to eq('failed')
+        expect(commit.yaml_errors).to_not be_nil
       end
 
       describe :ci_skip? do
@@ -100,7 +117,7 @@ module Ci
         end
 
         it "skips builds creation if there is [ci skip] tag in commit message and yaml is invalid" do
-          stub_ci_commit_yaml_file('invalid: file')
+          stub_ci_commit_yaml_file('invalid: file: fiile')
           commits = [{ message: message }]
           commit = service.execute(project, user,
                                    ref: 'refs/tags/0_1',
@@ -110,6 +127,7 @@ module Ci
           )
           expect(commit.builds.any?).to be false
           expect(commit.status).to eq("skipped")
+          expect(commit.yaml_errors).to be_nil
         end
       end