Polish CreatePipelineService and add spec

7 files changed, 121 insertions, 48 deletions

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 1ce5f0c6fd2..189541af6d7 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -1,8 +1,8 @@
 module Ci
   class CreatePipelineService < BaseService
-    attr_reader :pipeline, :trigger_request
+    attr_reader :pipeline
 
-    def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil)
+    def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, &block)
       @pipeline = Ci::Pipeline.new(
         source: source,
         project: project,
@@ -15,15 +15,25 @@ module Ci
         pipeline_schedule: schedule
       )
 
-      @trigger_request = trigger_request
+      result = validate(trigger_request,
+                        ignore_skip_ci: ignore_skip_ci,
+                        save_on_errors: save_on_errors)
 
-      return error if error = validate_parameter
+      return result if result
 
-      if !ignore_skip_ci && skip_ci?
-        pipeline.skip if save_on_errors
-        return pipeline
-      end
+      err = create_pipeline(&block)
+      return err if err
+
+      cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
+
+      pipeline_created_counter.increment(source: source)
+
+      pipeline.tap(&:process!)
+    end
+
+    private
 
+    def create_pipeline
       Ci::Pipeline.transaction do
         update_merge_requests_head_pipeline if pipeline.save
 
@@ -34,16 +44,12 @@ module Ci
           .execute(pipeline)
       end
 
-      cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
-
-      pipeline_created_counter.increment(source: source)
-
-      pipeline.tap(&:process!)
+      return nil
+    rescue ActiveRecord::RecordInvalid => invalid
+      return error('Failed to persist the pipeline')
     end
 
-    private
-
-    def validate_parameter
+    def validate(trigger_request, ignore_skip_ci:, save_on_errors:)
       unless project.builds_enabled?
         return error('Pipeline is disabled')
       end
@@ -67,6 +73,11 @@ module Ci
         return error(pipeline.yaml_errors, save: save_on_errors)
       end
 
+      if !ignore_skip_ci && skip_ci?
+        pipeline.skip if save_on_errors
+        return pipeline
+      end
+
       unless pipeline.has_stage_seeds?
         return error('No stages / jobs for this pipeline.')
       end
diff --git a/app/services/ci/pipeline_trigger_service.rb b/app/services/ci/pipeline_trigger_service.rb
index f72cadd6bb9..f332e1b9e7d 100644
--- a/app/services/ci/pipeline_trigger_service.rb
+++ b/app/services/ci/pipeline_trigger_service.rb
@@ -14,7 +14,7 @@ module Ci
 
       pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: params[:ref])
         .execute(:trigger, ignore_skip_ci: true) do |pipeline|
-          pipeline.variables.create!(params[:variables])
+          pipeline.variables.create!(params[:variables]) if params[:variables]
         end
 
       if pipeline.persisted?
diff --git a/spec/factories/ci/pipeline_variable_variables.rb b/spec/factories/ci/pipeline_variable_variables.rb
new file mode 100644
index 00000000000..7c1a7faec08
--- /dev/null
+++ b/spec/factories/ci/pipeline_variable_variables.rb
@@ -0,0 +1,8 @@
+FactoryGirl.define do
+  factory :ci_pipeline_variable, class: Ci::PipelineVariable do
+    sequence(:key) { |n| "VARIABLE_#{n}" }
+    value 'VARIABLE_VALUE'
+
+    pipeline factory: :ci_empty_pipeline
+  end
+end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 0b521d720f3..2ca78b60e91 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1468,6 +1468,12 @@ describe Ci::Build, :models do
       it { is_expected.to include(predefined_trigger_variable) }
     end
 
+    context 'when pipeline has a variable' do
+      let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) }
+
+      it { is_expected.to include(pipeline_variable.to_runner_variable) }
+    end
+
     context 'when a job was triggered by a pipeline schedule' do
       let(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project) }
 
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index ba0696fa210..099ccf31449 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -17,6 +17,7 @@ describe Ci::Pipeline, models: true do
 
   it { is_expected.to have_many(:statuses) }
   it { is_expected.to have_many(:trigger_requests) }
+  it { is_expected.to have_many(:variables) }
   it { is_expected.to have_many(:builds) }
   it { is_expected.to have_many(:auto_canceled_pipelines) }
   it { is_expected.to have_many(:auto_canceled_jobs) }
diff --git a/spec/models/ci/pipeline_variable_spec.rb b/spec/models/ci/pipeline_variable_spec.rb
new file mode 100644
index 00000000000..2ce78e34b0c
--- /dev/null
+++ b/spec/models/ci/pipeline_variable_spec.rb
@@ -0,0 +1,8 @@
+require 'spec_helper'
+
+describe Ci::PipelineVariable, models: true do
+  subject { build(:ci_pipeline_variable) }
+
+  it { is_expected.to include_module(HasVariable) }
+  it { is_expected.to validate_uniqueness_of(:key).scoped_to(:pipeline_id) }
+end
diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb
index b7f2c36d936..7cad40e8dd3 100644
--- a/spec/services/ci/pipeline_trigger_service_spec.rb
+++ b/spec/services/ci/pipeline_trigger_service_spec.rb
@@ -1,52 +1,91 @@
 require 'spec_helper'
 
-describe Ci::PipelineTriggerServiceService, services: true do
-  let(:service) { described_class.new }
+describe Ci::PipelineTriggerService, services: true do
   let(:project) { create(:project, :repository) }
-  let(:trigger) { create(:ci_trigger, project: project) }
 
   before do
     stub_ci_pipeline_to_return_yaml_file
   end
 
   describe '#execute' do
-    context 'valid params' do
-      subject { service.execute(project, trigger, 'master') }
-
-      context 'without owner' do
-        it { expect(subject).to be_kind_of(Ci::TriggerRequest) }
-        it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) }
-        it { expect(subject.pipeline).to be_trigger }
-        it { expect(subject.builds.first).to be_kind_of(Ci::Build) }
-      end
+    let(:user) { create(:user) }
+
+    before do
+      project.add_developer(user)
+    end
+
+    let(:result) { described_class.new(project, user, params).execute }
+    let(:trigger) { create(:ci_trigger, project: project, owner: user) }
+
+    context 'when params have an existsed trigger token' do
+      let(:token) { trigger.token }
+
+      context 'when params have an existsed ref' do
+        let(:ref) { 'master' }
+
+        it 'triggers a pipeline' do
+          expect { result }.to change { Ci::Pipeline.count }.by(1)
+          expect(result[:pipeline].ref).to eq(ref)
+          expect(result[:pipeline].project).to eq(project)
+          expect(result[:pipeline].user).to eq(trigger.owner)
+          expect(result[:status]).to eq(:success)
+        end
+
+        context 'when params have a variable' do
+          let(:variables) { [{ key: 'AAA', value: 'AAA123' }] }
 
-      context 'with owner' do
-        let(:owner) { create(:user) }
-        let(:trigger) { create(:ci_trigger, project: project, owner: owner) }
+          it 'has a variable' do
+            expect { result }.to change { Ci::PipelineVariable.count }.by(1)
+            expect(result[:pipeline].variables.first.key).to eq(variables.first[:key])
+            expect(result[:pipeline].variables.first.value).to eq(variables.first[:value])
+          end
+        end
 
-        it { expect(subject).to be_kind_of(Ci::TriggerRequest) }
-        it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) }
-        it { expect(subject.pipeline).to be_trigger }
-        it { expect(subject.pipeline.user).to eq(owner) }
-        it { expect(subject.builds.first).to be_kind_of(Ci::Build) }
-        it { expect(subject.builds.first.user).to eq(owner) }
+        context 'when params have two variables' do
+          let(:variables) { [{ key: 'AAA', value:'AAA123' }, { key: 'BBB', value:'BBB123' }] }
+
+          it 'has two variables' do
+            expect { result }.to change { Ci::PipelineVariable.count }.by(2)
+            expect(result[:pipeline].variables.first.key).to eq(variables.first[:key])
+            expect(result[:pipeline].variables.first.value).to eq(variables.first[:value])
+            expect(result[:pipeline].variables.last.key).to eq(variables.last[:key])
+            expect(result[:pipeline].variables.last.value).to eq(variables.last[:value])
+          end
+        end
+
+        context 'when params have two variables and keys are duplicated' do
+          let(:variables) { [{ key: 'AAA', value:'AAA123' }, { key: 'AAA', value:'BBB123' }] }
+
+          it 'returns error' do
+            expect { result }.not_to change { Ci::Pipeline.count }
+            expect(result[:http_status]).to eq(400)
+          end
+        end
       end
-    end
 
-    context 'no commit for ref' do
-      subject { service.execute(project, trigger, 'other-branch') }
+      context 'when params have a non-existsed ref' do
+        let(:ref) { 'invalid-ref' }
 
-      it { expect(subject).to be_nil }
+        it 'does not trigger a pipeline' do
+          expect { result }.not_to change { Ci::Pipeline.count }
+          expect(result[:http_status]).to eq(400)
+        end
+      end
     end
 
-    context 'no builds created' do
-      subject { service.execute(project, trigger, 'master') }
+    context 'when params have a non-existsed trigger token' do
+      let(:token) { 'invalid-token' }
 
-      before do
-        stub_ci_pipeline_yaml_file('script: { only: [develop], script: hello World }')
+      it 'does not trigger a pipeline' do
+        expect { result }.not_to change { Ci::Pipeline.count }
+        expect(result).to be_nil
       end
-
-      it { expect(subject).to be_nil }
     end
   end
+
+  def params
+    { token: defined?(token) ? token : nil,
+      ref: defined?(ref) ? ref : nil,
+      variables: defined?(variables) ? variables : nil }
+  end
 end