From 2b907f61ff5db3ff68b27a9d3bb164745ab7703b Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 18 Nov 2015 16:32:00 +0100 Subject: Commits without .gitlab-ci.yml are marked as skipped - Save detailed error when YAML syntax --- app/models/ci/commit.rb | 5 ++++- spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 6 +++++- spec/services/ci/create_commit_service_spec.rb | 20 +++++++++++++++++++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb index 33b57173928..73c1570c212 100644 --- a/app/models/ci/commit.rb +++ b/app/models/ci/commit.rb @@ -188,12 +188,15 @@ 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 save_yaml_error(e.message) nil + rescue 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") nil end diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index 7d90f9877c6..6f287719ba6 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/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 diff --git a/spec/services/ci/create_commit_service_spec.rb b/spec/services/ci/create_commit_service_spec.rb index e3a8fe9681b..392f5fce35f 100644 --- a/spec/services/ci/create_commit_service_spec.rb +++ b/spec/services/ci/create_commit_service_spec.rb @@ -100,7 +100,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 +110,24 @@ module Ci ) expect(commit.builds.any?).to be false expect(commit.status).to eq("skipped") + expect(commit.yaml_errors).to be_nil + end + end + + describe :config_processor do + it "skips builds creation if yaml is invalid" do + 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("skipped") + expect(commit.yaml_errors).to_not be_nil end end -- cgit v1.2.1 From 0df7a32ea50baf251f03e6bfc5b91c5ccb68aad0 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 19 Nov 2015 12:08:30 +0100 Subject: Fix tests --- app/models/ci/commit.rb | 2 +- spec/services/ci/create_commit_service_spec.rb | 38 +++++++++++++------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb index 73c1570c212..b0c78499e49 100644 --- a/app/models/ci/commit.rb +++ b/app/models/ci/commit.rb @@ -196,7 +196,7 @@ module Ci rescue Psych::SyntaxError => e save_yaml_error(e.message) nil - rescue Exception => e + rescue Exception save_yaml_error("Undefined yaml error") nil end diff --git a/spec/services/ci/create_commit_service_spec.rb b/spec/services/ci/create_commit_service_spec.rb index 392f5fce35f..58d81c46a6d 100644 --- a/spec/services/ci/create_commit_service_spec.rb +++ b/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(commit.yaml_errors).to_not 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('skipped') + expect(commit.yaml_errors).to_not be_nil end describe :ci_skip? do @@ -114,23 +131,6 @@ module Ci end end - describe :config_processor do - it "skips builds creation if yaml is invalid" do - 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("skipped") - expect(commit.yaml_errors).to_not be_nil - end - end - it "skips build creation if there are already builds" do allow_any_instance_of(Ci::Commit).to receive(:ci_yaml_file) { gitlab_ci_yaml } -- cgit v1.2.1 From fd6b58949652c5fb9925c061fae823da7e468ca2 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 19 Nov 2015 12:09:24 +0100 Subject: Since GitLab CI is enabled by default, remove enabling it by pushing .gitlab-ci.yml --- CHANGELOG | 1 + app/services/git_push_service.rb | 12 ------------ 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ea8d5d6000f..01f18dea23b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,6 +15,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 diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index ccb6b97858c..f11690aa3f4 100644 --- a/app/services/git_push_service.rb +++ b/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 -- cgit v1.2.1 From a5b10196e649b57cf7cc698fe6fb34f73eec47ea Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 19 Nov 2015 15:56:03 +0100 Subject: Fix tests --- spec/services/ci/create_commit_service_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/ci/create_commit_service_spec.rb b/spec/services/ci/create_commit_service_spec.rb index 58d81c46a6d..e0ede1d58b7 100644 --- a/spec/services/ci/create_commit_service_spec.rb +++ b/spec/services/ci/create_commit_service_spec.rb @@ -64,7 +64,7 @@ module Ci expect(result).to be_persisted expect(result.builds.any?).to be_falsey expect(result.status).to eq('skipped') - expect(commit.yaml_errors).to_not be_nil + expect(result.yaml_errors).to be_nil end it 'skips commits if yaml is invalid' do @@ -79,7 +79,7 @@ module Ci commits: commits ) expect(commit.builds.any?).to be false - expect(commit.status).to eq('skipped') + expect(commit.status).to eq('failed') expect(commit.yaml_errors).to_not be_nil end -- cgit v1.2.1 From 8248314bc9256d3a0252ad6322df098edca7385a Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 19 Nov 2015 20:16:56 +0100 Subject: Don't rescue Exception, but StandardError --- app/controllers/ci/lints_controller.rb | 4 ++-- app/models/ci/commit.rb | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/controllers/ci/lints_controller.rb b/app/controllers/ci/lints_controller.rb index 24dd1b5c93a..a4f6aff49b4 100644 --- a/app/controllers/ci/lints_controller.rb +++ b/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 diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb index b0c78499e49..971e899de84 100644 --- a/app/models/ci/commit.rb +++ b/app/models/ci/commit.rb @@ -190,14 +190,11 @@ module Ci 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 Psych::SyntaxError => e - save_yaml_error(e.message) - nil - rescue Exception - save_yaml_error("Undefined yaml error") + rescue + save_yaml_error("Undefined error") nil end -- cgit v1.2.1