From af2f56f8f742e00ddb298fadea763fd0fe7054f0 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 2 Jun 2016 12:39:15 +0200
Subject: [PATCH] Refactor ci commit pipeline to prevent implicit saves

---
 app/models/ci/commit.rb                       | 16 +++--------
 app/services/ci/create_pipeline_service.rb    | 28 +++++++++----------
 app/services/create_commit_builds_service.rb  |  2 +-
 .../create_commit_builds_service_spec.rb      |  2 +-
 4 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/app/models/ci/commit.rb b/app/models/ci/commit.rb
index f22b573a94..c682f3e570 100644
--- a/app/models/ci/commit.rb
+++ b/app/models/ci/commit.rb
@@ -13,7 +13,7 @@ module Ci
     validate :valid_commit_sha
 
     # Invalidate object and save if when touched
-    after_touch :update_state
+    after_touch :update_state!
 
     def self.truncate_sha(sha)
       sha[0...8]
@@ -135,10 +135,10 @@ module Ci
       @config_processor ||= begin
         Ci::GitlabCiYamlProcessor.new(ci_yaml_file, project.path_with_namespace)
       rescue Ci::GitlabCiYamlProcessor::ValidationError, Psych::SyntaxError => e
-        save_yaml_error(e.message)
+        self.yaml_errors = (e.message)
         nil
       rescue
-        save_yaml_error("Undefined error")
+        self.yaml_errors = 'Undefined error'
         nil
       end
     end
@@ -159,9 +159,7 @@ module Ci
       git_commit_message =~ /(\[ci skip\])/ if git_commit_message
     end
 
-    private
-
-    def update_state
+    def update_state!
       statuses.reload
       self.status = if yaml_errors.blank?
                       statuses.latest.status || 'skipped'
@@ -173,11 +171,5 @@ module Ci
       self.duration = statuses.latest.duration
       save
     end
-
-    def save_yaml_error(error)
-      return if self.yaml_errors?
-      self.yaml_errors = error
-      update_state
-    end
   end
 end
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 5bc0c31cb4..0336b767de 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -8,7 +8,9 @@ module Ci
         return pipeline
       end
 
-      unless commit
+      if commit
+        pipeline.sha = commit.id
+      else
         pipeline.errors.add(:base, 'Commit not found')
         return pipeline
       end
@@ -18,22 +20,18 @@ module Ci
         return pipeline
       end
 
-      begin
-        Ci::Commit.transaction do
-          pipeline.sha = commit.id
-
-          unless pipeline.config_processor
-            pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
-            raise ActiveRecord::Rollback
-          end
-
-          pipeline.save!
-          pipeline.create_builds(current_user)
-        end
-      rescue
-        pipeline.errors.add(:base, 'The pipeline could not be created. Please try again.')
+      unless pipeline.config_processor
+        pipeline.errors.add(:base, pipeline.yaml_errors || 'Missing .gitlab-ci.yml file')
+        return pipeline
       end
 
+      pipeline.save!
+
+      unless pipeline.create_builds(current_user)
+        pipeline.errors.add(:base, 'No builds for this pipeline.')
+      end
+
+      pipeline.update_state!
       pipeline
     end
 
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index 5b6fefe669..ee84023e51 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -34,7 +34,7 @@ class CreateCommitBuildsService
       commit.create_builds(user)
     end
 
-    commit.touch
+    commit.update_state!
     commit
   end
 end
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index 9ae8f31b37..e643991e0b 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -81,7 +81,7 @@ describe CreateCommitBuildsService, services: true do
       expect(commit.yaml_errors).not_to be_nil
     end
 
-    describe :ci_skip? do
+    context 'when commit contains a [ci skip] directive' do
       let(:message) { "some message[ci skip]" }
 
       before do