Improve code clarity in pipeline create service

3 files changed, 41 insertions, 17 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 83d683b63e4..63639ff2c1f 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -100,7 +100,7 @@ module Ci
 
     def create_builds(user, trigger_request = nil)
       build_builds(user, 'success', trigger_request)
-      save!
+      save
     end
 
     def create_next_builds(build)
diff --git a/app/services/create_commit_builds_service.rb b/app/services/create_commit_builds_service.rb
index f2a537c595e..668d0a86549 100644
--- a/app/services/create_commit_builds_service.rb
+++ b/app/services/create_commit_builds_service.rb
@@ -14,26 +14,50 @@ class CreateCommitBuildsService
       return false
     end
 
-    pipeline = Ci::Pipeline.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
+    @pipeline = Ci::Pipeline.new(project: project, sha: sha, ref: ref, before_sha: before_sha, tag: tag)
 
-    # Skip creating pipeline when no gitlab-ci.yml is found
-    unless pipeline.ci_yaml_file
-      return pipeline
+    ##
+    # Skip creating pipeline if no gitlab-ci.yml is found
+    #
+    unless @pipeline.ci_yaml_file
+      return false
     end
 
+    ##
     # Skip creating builds for commits that have [ci skip]
-   if !pipeline.skip_ci? && pipeline.config_processor
-      # Create builds for commit
-      unless pipeline.build_builds(user)
-        pipeline.errors.add(:base, 'No builds created')
-        return pipeline
-      end
+    # but save pipeline object
+    #
+    if @pipeline.skip_ci?
+      return save_pipeline!
+    end
+
+    ##
+    # Skip creating builds when CI config is invalid
+    # but save pipeline object
+    #
+    unless @pipeline.config_processor
+      return save_pipeline!
+    end
+
+    ##
+    # Skip creating pipeline object if there are no builds for it.
+    #
+    unless @pipeline.build_builds(user)
+      @pipeline.errors.add(:base, 'No builds created')
+      return false
     end
 
-    # Create a new pipeline
-    pipeline.save!
+    save_pipeline!
+  end
+
+  private
 
-    pipeline.touch
-    pipeline
+  ##
+  # Create a new pipeline and touch object to calculate status
+  #
+  def save_pipeline!
+    @pipeline.save!
+    @pipeline.touch
+    @pipeline
   end
 end
diff --git a/spec/services/create_commit_builds_service_spec.rb b/spec/services/create_commit_builds_service_spec.rb
index 08cbc9beb5c..50ce9659c10 100644
--- a/spec/services/create_commit_builds_service_spec.rb
+++ b/spec/services/create_commit_builds_service_spec.rb
@@ -60,7 +60,7 @@ describe CreateCommitBuildsService, services: true do
                                after: '31das312',
                                commits: [{ message: 'Message' }]
                               )
-      expect(result).not_to be_persisted
+      expect(result).to be_falsey
       expect(Ci::Pipeline.count).to eq(0)
     end
 
@@ -184,7 +184,7 @@ describe CreateCommitBuildsService, services: true do
                                  before: '00000000',
                                  after: '31das312',
                                  commits: [{ message: 'some msg' }])
-        expect(result).not_to be_persisted
+        expect(result).to be_falsey
         expect(Ci::Build.all).to be_empty
         expect(Ci::Pipeline.count).to eq(0)
       end