Merge branch 'feature/sm/35282-ci_pipeline_variables-2-ee' into 'master'

Port "Follow-up from "Allow to use cross project pipelines" ver 2" to EE

See merge request !2555

app/models/ci/build.rb

@@ -223,6 +223,7 @@ module Ci
       variables += project.group.secret_variables_for(ref, project).map(&:to_runner_variable) if project.group
       variables += secret_variables(environment: environment)
       variables += trigger_request.user_variables if trigger_request
+      variables += pipeline.variables.map(&:to_runner_variable)
       variables += pipeline.pipeline_schedule.job_variables if pipeline.pipeline_schedule
       variables += persisted_environment_variables if environment
 

app/models/ci/pipeline.rb

@@ -27,6 +27,7 @@ module Ci
     has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id
     has_many :builds, foreign_key: :commit_id
     has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id # rubocop:disable Cop/ActiveRecordDependent
+    has_many :variables, class_name: 'Ci::PipelineVariable'
 
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.

app/models/ci/pipeline_variable.rb

+module Ci
+  class PipelineVariable < ActiveRecord::Base
+    extend Ci::Model
+    include HasVariable
+
+    belongs_to :pipeline
+
+    validates :key, uniqueness: { scope: :pipeline_id }
+  end
+end

app/services/ci/create_pipeline_service.rb

@@ -2,7 +2,7 @@ module Ci
   class CreatePipelineService < BaseService
     attr_reader :pipeline
 
-    def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, mirror_update: false, &block)
+    def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, mirror_update: false)
       @pipeline = Ci::Pipeline.new(
         source: source,
         project: project,
@@ -22,7 +22,27 @@ module Ci
 
       return result if result
 
-      _create_pipeline(source, &block)
+      begin
+        Ci::Pipeline.transaction do
+          pipeline.save!
+
+          yield(pipeline) if block_given?
+
+          Ci::CreatePipelineStagesService
+            .new(project, current_user)
+            .execute(pipeline)
+        end
+      rescue ActiveRecord::RecordInvalid => e
+        return error("Failed to persist the pipeline: #{e}")
+      end
+
+      update_merge_requests_head_pipeline
+
+      cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
+
+      pipeline_created_counter.increment(source: source)
+
+      pipeline.tap(&:process!)
     end
 
     private
@@ -69,24 +89,6 @@ module Ci
       end
     end
 
-    def _create_pipeline(source)
-      Ci::Pipeline.transaction do
-        update_merge_requests_head_pipeline if pipeline.save
-
-        yield(pipeline) if block_given?
-
-        Ci::CreatePipelineStagesService
-          .new(project, current_user)
-          .execute(pipeline)
-      end
-
-      cancel_pending_pipelines if project.auto_cancel_pending_pipelines?
-
-      pipeline_created_counter.increment(source: source)
-
-      pipeline.tap(&:process!)
-    end
-
     def allowed_to_trigger_pipeline?(triggering_user)
       if triggering_user
         allowed_to_create?(triggering_user)

app/services/ci/create_trigger_request_service.rb

+# This class is deprecated because we're closing Ci::TriggerRequest.
+# New class is PipelineTriggerService (app/services/ci/pipeline_trigger_service.rb)
+# which is integrated with Ci::PipelineVariable instaed of Ci::TriggerRequest.
+# We remove this class after we removed v1 and v3 API. This class is still being
+# referred by such legacy code.
 module Ci
   module CreateTriggerRequestService
     Result = Struct.new(:trigger_request, :pipeline)

app/services/ci/pipeline_trigger_service.rb

@@ -14,9 +14,11 @@ module Ci
       # this check is to not leak the presence of the project if user cannot read it
       return unless trigger.project == project
 
-      trigger_request = trigger.trigger_requests.create(variables: params[:variables])
       pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: params[:ref])
-        .execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request)
+        .execute(:trigger, ignore_skip_ci: true) do |pipeline|
+          trigger.trigger_requests.create!(pipeline: pipeline)
+          create_pipeline_variables!(pipeline)
+        end
 
       if pipeline.persisted?
         success(pipeline: pipeline)
@@ -47,17 +49,27 @@ module Ci
         error(pipeline.errors.messages, 400)
       end
     end
-    
+
     def trigger_from_token
       return @trigger if defined?(@trigger)
-      
+
       @trigger = Ci::Trigger.find_by_token(params[:token].to_s)
     end
 
     def job_from_token
       return @job if defined?(@job)
-      
+
       @job = Ci::Build.find_by_token(params[:token].to_s)
     end
+
+    def create_pipeline_variables!(pipeline)
+      return unless params[:variables]
+
+      variables = params[:variables].map do |key, value|
+        { key: key, value: value }
+      end
+
+      pipeline.variables.create!(variables)
+    end
   end
 end

db/migrate/20170720130522_create_ci_pipeline_variables.rb

+class CreateCiPipelineVariables < ActiveRecord::Migration
+  DOWNTIME = false
+
+  def up
+    create_table :ci_pipeline_variables do |t|
+      t.string :key, null: false
+      t.text :value
+      t.text :encrypted_value
+      t.string :encrypted_value_salt
+      t.string :encrypted_value_iv
+      t.integer :pipeline_id, null: false
+    end
+
+    add_index :ci_pipeline_variables, [:pipeline_id, :key], unique: true
+  end
+
+  def down
+    drop_table :ci_pipeline_variables
+  end
+end

db/migrate/20170720130749_add_foreign_key_to_ci_pipeline_variables.rb

+class AddForeignKeyToCiPipelineVariables < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key(:ci_pipeline_variables, :ci_pipelines, column: :pipeline_id)
+  end
+
+  def down
+    remove_foreign_key(:ci_pipeline_variables, column: :pipeline_id)
+  end
+end

db/schema.rb

@@ -357,6 +357,17 @@ ActiveRecord::Schema.define(version: 20170726111039) do
   add_index "ci_pipeline_schedules", ["next_run_at", "active"], name: "index_ci_pipeline_schedules_on_next_run_at_and_active", using: :btree
   add_index "ci_pipeline_schedules", ["project_id"], name: "index_ci_pipeline_schedules_on_project_id", using: :btree
 
+  create_table "ci_pipeline_variables", force: :cascade do |t|
+    t.string "key", null: false
+    t.text "value"
+    t.text "encrypted_value"
+    t.string "encrypted_value_salt"
+    t.string "encrypted_value_iv"
+    t.integer "pipeline_id", null: false
+  end
+
+  add_index "ci_pipeline_variables", ["pipeline_id", "key"], name: "index_ci_pipeline_variables_on_pipeline_id_and_key", unique: true, using: :btree
+
   create_table "ci_pipelines", force: :cascade do |t|
     t.string "ref"
     t.string "sha"
@@ -1907,6 +1918,7 @@ ActiveRecord::Schema.define(version: 20170726111039) do
   add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify
+  add_foreign_key "ci_pipeline_variables", "ci_pipelines", column: "pipeline_id", name: "fk_f29c5f4380", on_delete: :cascade
   add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify
   add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify
   add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade

spec/factories/ci/pipeline_variable_variables.rb

+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

spec/lib/gitlab/import_export/all_models.yml

@@ -106,6 +106,7 @@ pipelines:
 - statuses
 - builds
 - trigger_requests
+- variables
 - auto_canceled_by
 - auto_canceled_pipelines
 - auto_canceled_jobs
@@ -120,6 +121,8 @@ pipelines:
 - sourced_pipelines
 - triggered_by_pipeline
 - triggered_pipelines
+pipeline_variables:
+- pipeline
 stages:
 - project
 - pipeline

spec/models/ci/build_spec.rb

@@ -1487,6 +1487,12 @@ describe Ci::Build 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) }
 

spec/models/ci/pipeline_spec.rb

@@ -17,6 +17,7 @@ describe Ci::Pipeline 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) }

spec/models/ci/pipeline_variable_spec.rb

+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

spec/requests/api/triggers_spec.rb

@@ -22,6 +22,7 @@ describe API::Triggers do
 
     before do
       stub_ci_pipeline_to_return_yaml_file
+      trigger.update(owner: user)
     end
 
     context 'Handles errors' do
@@ -55,8 +56,7 @@ describe API::Triggers do
         post api("/projects/#{project.id}/trigger/pipeline"), options.merge(ref: 'other-branch')
 
         expect(response).to have_http_status(400)
-        expect(json_response['message']['base'])
-          .to contain_exactly('Reference not found')
+        expect(json_response['message']).to eq('base' => ["Reference not found"])
       end
 
       context 'Validates variables' do
@@ -82,7 +82,7 @@ describe API::Triggers do
           post api("/projects/#{project.id}/trigger/pipeline"), options.merge(variables: variables, ref: 'master')
 
           expect(response).to have_http_status(201)
-          expect(pipeline.builds.reload.first.trigger_request.variables).to eq(variables)
+          expect(pipeline.variables.map { |v| { v.key => v.value } }.last).to eq(variables)
         end
       end
     end

spec/services/ci/pipeline_trigger_service_spec.rb

+require 'spec_helper'
+
+describe Ci::PipelineTriggerService, services: true do
+  let(:project) { create(:project, :repository) }
+
+  before do
+    stub_ci_pipeline_to_return_yaml_file
+  end
+
+  describe '#execute' do
+    let(:user) { create(:user) }
+    let(:trigger) { create(:ci_trigger, project: project, owner: user) }
+    let(:result) { described_class.new(project, user, params).execute }
+
+    before do
+      project.add_developer(user)
+    end
+
+    context 'when trigger belongs to a different project' do
+      let(:params) { { token: trigger.token, ref: 'master', variables: nil } }
+      let(:trigger) { create(:ci_trigger, project: create(:empty_project), owner: user) }
+
+      it 'does nothing' do
+        expect { result }.not_to change { Ci::Pipeline.count }
+      end
+    end
+
+    context 'when params have an existsed trigger token' do
+      context 'when params have an existsed ref' do
+        let(:params) { { token: trigger.token, ref: 'master', variables: nil } }
+
+        it 'triggers a pipeline' do
+          expect { result }.to change { Ci::Pipeline.count }.by(1)
+          expect(result[:pipeline].ref).to eq('master')
+          expect(result[:pipeline].project).to eq(project)
+          expect(result[:pipeline].user).to eq(trigger.owner)
+          expect(result[:status]).to eq(:success)
+        end
+
+        context 'when commit message has [ci skip]' do
+          before do
+            allow_any_instance_of(Ci::Pipeline).to receive(:git_commit_message) { '[ci skip]' }
+          end
+
+          it 'ignores [ci skip] and create as general' do
+            expect { result }.to change { Ci::Pipeline.count }.by(1)
+            expect(result[:status]).to eq(:success)
+          end
+        end
+
+        context 'when params have a variable' do
+          let(:params) { { token: trigger.token, ref: 'master', variables: variables } }
+          let(:variables) { { 'AAA' => 'AAA123' } }
+
+          it 'has a variable' do
+            expect { result }.to change { Ci::PipelineVariable.count }.by(1)
+                             .and change { Ci::TriggerRequest.count }.by(1)
+            expect(result[:pipeline].variables.map { |v| { v.key => v.value } }.first).to eq(variables)
+            expect(result[:pipeline].trigger_requests.last.variables).to be_nil
+          end
+        end
+      end
+
+      context 'when params have a non-existsed ref' do
+        let(:params) { { token: trigger.token, ref: 'invalid-ref', variables: 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 'when params have a non-existsed trigger token' do
+      let(:params) { { token: 'invalid-token', ref: nil, variables: nil } }
+
+      it 'does not trigger a pipeline' do
+        expect { result }.not_to change { Ci::Pipeline.count }
+        expect(result).to be_nil
+      end
+    end
+  end
+end