Refine implementation of pipeline stage seeds

12 files changed, 172 insertions, 205 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index e64e16657fe..fb1d4720ba8 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -11,9 +11,7 @@ module Ci
     belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline'
     belongs_to :pipeline_schedule, class_name: 'Ci::PipelineSchedule'
 
-    has_many :auto_canceled_pipelines, class_name: 'Ci::Pipeline', foreign_key: 'auto_canceled_by_id'
-    has_many :auto_canceled_jobs, class_name: 'CommitStatus', foreign_key: 'auto_canceled_by_id'
-
+    has_many :stages
     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
@@ -28,6 +26,9 @@ module Ci
     has_many :manual_actions, -> { latest.manual_actions }, foreign_key: :commit_id, class_name: 'Ci::Build'
     has_many :artifacts, -> { latest.with_artifacts_not_expired }, foreign_key: :commit_id, class_name: 'Ci::Build'
 
+    has_many :auto_canceled_pipelines, class_name: 'Ci::Pipeline', foreign_key: 'auto_canceled_by_id'
+    has_many :auto_canceled_jobs, class_name: 'CommitStatus', foreign_key: 'auto_canceled_by_id'
+
     delegate :id, to: :project, prefix: true
 
     validates :sha, presence: { unless: :importing? }
@@ -296,17 +297,13 @@ module Ci
     end
 
     def stage_seeds
-      return unless config_processor
-
-      seeds_scope = { ref: ref, tag: tag?, trigger: trigger_requests.first }
+      return [] unless config_processor
 
-      @seeds ||= config_processor.stage_seeds(seeds_scope).tap do |seeds|
-        seeds.pipeline = self
-      end
+      @stage_seeds ||= config_processor.stage_seeds(self)
     end
 
-    def has_stages?
-      stage_seeds&.has_stages?
+    def has_stage_seeds?
+      stage_seeds.any?
     end
 
     def has_warnings?
diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb
new file mode 100644
index 00000000000..59570924c8d
--- /dev/null
+++ b/app/models/ci/stage.rb
@@ -0,0 +1,11 @@
+module Ci
+  class Stage < ActiveRecord::Base
+    extend Ci::Model
+
+    belongs_to :project
+    belongs_to :pipeline
+
+    has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id
+    has_many :builds, foreign_key: :commit_id
+  end
+end
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index fe63728ea23..82f6ce8e484 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -5,10 +5,10 @@ class CommitStatus < ActiveRecord::Base
 
   self.table_name = 'ci_builds'
 
+  belongs_to :user
   belongs_to :project
   belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :commit_id
   belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline'
-  belongs_to :user
 
   delegate :commit, to: :pipeline
   delegate :sha, :short_sha, to: :pipeline
@@ -18,7 +18,7 @@ class CommitStatus < ActiveRecord::Base
   validates :name, presence: true
 
   alias_attribute :author, :user
-  
+
   scope :failed_but_allowed, -> do
     where(allow_failure: true, status: [:failed, :canceled])
   end
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 3f177122180..4dd74ebb1bb 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -42,7 +42,7 @@ module Ci
         return pipeline
       end
 
-      unless pipeline.has_stages?
+      if pipeline.has_stage_seeds?
         return error('No stages / jobs for this pipeline.')
       end
 
diff --git a/app/services/ci/create_pipeline_stages_service.rb b/app/services/ci/create_pipeline_stages_service.rb
index 95b55c37ec1..f2c175adee6 100644
--- a/app/services/ci/create_pipeline_stages_service.rb
+++ b/app/services/ci/create_pipeline_stages_service.rb
@@ -1,45 +1,20 @@
 module Ci
   class CreatePipelineStagesService < BaseService
-    attr_reader :pipeline
-
     def execute(pipeline)
-      @pipeline = pipeline
-
-      new_builds.map do |build_attributes|
-        create_build(build_attributes)
+      pipeline.stage_seeds.each do |seed|
+        seed.user = current_user
+
+        seed.create! do |build|
+          ##
+          # Create the environment before the build starts. This sets its slug and
+          # makes it available as an environment variable
+          #
+          if build.has_environment?
+            environment_name = build.expanded_environment_name
+            project.environments.find_or_create_by(name: environment_name)
+          end
+        end
       end
     end
-
-    delegate :project, to: :pipeline
-
-    private
-
-    def create_build(build_attributes)
-      build_attributes = build_attributes.merge(
-        pipeline: pipeline,
-        project: project,
-        ref: pipeline.ref,
-        tag: pipeline.tag,
-        user: current_user,
-        trigger_request: trigger_request
-      )
-
-      build = pipeline.builds.create(build_attributes)
-
-      # Create the environment before the build starts. This sets its slug and
-      # makes it available as an environment variable
-      project.environments.find_or_create_by(name: build.expanded_environment_name) if
-        build.has_environment?
-
-      build
-    end
-
-    def new_builds
-      @new_builds ||= pipeline.config_builds_attributes
-    end
-
-    def trigger_request
-      @trigger_request ||= pipeline.trigger_requests.first
-    end
   end
 end
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index 4b0e87a945b..22af2671b18 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -50,14 +50,17 @@ module Ci
       end
     end
 
-    def stage_seeds(ref:, tag: false, trigger: nil)
-      Gitlab::Ci::Stage::Seeds.new.tap do |seeds|
-        @stages.uniq.each do |stage|
-          builds = builds_for_stage_and_ref(stage, ref, tag, trigger)
+    def stage_seeds(pipeline)
+      trigger_request = pipeline.trigger_requests.first
 
-          seeds.append_stage(stage, builds) if builds.any?
-       end
+      seeds = @stages.uniq.map do |stage|
+        builds = builds_for_stage_and_ref(
+          stage, pipeline.ref, pipeline.tag?, trigger_request)
+
+        Gitlab::Ci::Stage::Seed.new(pipeline, stage, builds) if builds.any?
       end
+
+      seeds.compact
     end
 
     def build_attributes(name)
diff --git a/lib/gitlab/ci/stage/seed.rb b/lib/gitlab/ci/stage/seed.rb
new file mode 100644
index 00000000000..f81f9347b4d
--- /dev/null
+++ b/lib/gitlab/ci/stage/seed.rb
@@ -0,0 +1,49 @@
+module Gitlab
+  module Ci
+    module Stage
+      class Seed
+        attr_reader :pipeline
+        delegate :project, to: :pipeline
+
+        def initialize(pipeline, stage, jobs)
+          @pipeline = pipeline
+          @stage = { name: stage }
+          @jobs = jobs.to_a.dup
+        end
+
+        def user=(current_user)
+          @jobs.map! do |attributes|
+            attributes.merge(user: current_user)
+          end
+        end
+
+        def stage
+          @stage.merge(project: project)
+        end
+
+        def builds
+          trigger = pipeline.trigger_requests.first
+
+          @jobs.map do |attributes|
+            attributes.merge(project: project,
+                             ref: pipeline.ref,
+                             tag: pipeline.tag,
+                             trigger_request: trigger)
+          end
+        end
+
+        def create!
+          pipeline.stages.create!(stage).tap do |stage|
+            builds_attributes = builds.map do |attributes|
+              attributes.merge(stage_id: stage.id)
+            end
+
+            pipeline.builds.create!(builds_attributes).each do |build|
+              yield build if block_given?
+            end
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/stage/seeds.rb b/lib/gitlab/ci/stage/seeds.rb
deleted file mode 100644
index cb5174a166b..00000000000
--- a/lib/gitlab/ci/stage/seeds.rb
+++ /dev/null
@@ -1,62 +0,0 @@
-module Gitlab
-  module Ci
-    module Stage
-      class Seeds
-        Seed = Struct.new(:stage, :jobs)
-
-        def initialize
-          @stages = []
-        end
-
-        def has_stages?
-          @stages.any?
-        end
-
-        def stages
-          @stages.map(&:stage)
-        end
-
-        def jobs
-          @stages.map(&:jobs).flatten
-        end
-
-        def append_stage(stage, jobs)
-          @stages << Seed.new({ name: stage }, jobs)
-        end
-
-        def pipeline=(pipeline)
-          trigger_request = pipeline.trigger_requests.first
-
-          stages.each do |attributes|
-            attributes.merge!(
-              pipeline: pipeline,
-              project: pipeline.project,
-            )
-          end
-
-          jobs.each do |attributes|
-            attributes.merge!(
-              pipeline: pipeline,
-              project: pipeline.project,
-              ref: pipeline.ref,
-              tag: pipeline.tag,
-              trigger_request: trigger_request
-            )
-          end
-        end
-
-        def user=(current_user)
-          jobs.each do |attributes|
-            attributes.merge!(user: current_user)
-          end
-        end
-
-        def to_attributes
-          @stages.map do |seed|
-            seed.stage.merge(builds_attributes: seed.jobs)
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index 7f652c17ed5..72b9cde10e7 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -91,15 +91,17 @@ module Ci
                     spinach: { stage: 'test', script: 'spinach' })
         end
 
-        it 'returns correctly fabricated stage seeds object' do
-          seeds = subject.stage_seeds(ref: 'master')
+        let(:pipeline) { create(:ci_empty_pipeline) }
 
-          expect(seeds.stages.size).to eq 2
-          expect(seeds.stages.dig(0, :name)).to eq 'test'
-          expect(seeds.stages.dig(1, :name)).to eq 'deploy'
-          expect(seeds.jobs.dig(0, :name)).to eq 'rspec'
-          expect(seeds.jobs.dig(1, :name)).to eq 'spinach'
-          expect(seeds.jobs.dig(2, :name)).to eq 'production'
+        it 'correctly fabricates a stage seeds object' do
+          seeds = subject.stage_seeds(pipeline)
+
+          expect(seeds.size).to eq 2
+          expect(seeds.first.stage[:name]).to eq 'test'
+          expect(seeds.second.stage[:name]).to eq 'deploy'
+          expect(seeds.first.builds.dig(0, :name)).to eq 'rspec'
+          expect(seeds.first.builds.dig(1, :name)).to eq 'spinach'
+          expect(seeds.second.builds.dig(0, :name)).to eq 'production'
         end
       end
 
@@ -109,12 +111,16 @@ module Ci
                     spinach: { stage: 'test', script: 'spinach', only: ['tags'] })
         end
 
+        let(:pipeline) do
+          create(:ci_empty_pipeline, ref: 'feature', tag: true)
+        end
+
         it 'returns stage seeds only assigned to master to master' do
-          seeds = subject.stage_seeds(ref: 'feature', tag: true)
+          seeds = subject.stage_seeds(pipeline)
 
-          expect(seeds.stages.size).to eq 1
-          expect(seeds.stages.dig(0, :name)).to eq 'test'
-          expect(seeds.jobs.dig(0, :name)).to eq 'spinach'
+          expect(seeds.size).to eq 1
+          expect(seeds.first.stage[:name]).to eq 'test'
+          expect(seeds.first.builds.dig(0, :name)).to eq 'spinach'
         end
       end
     end
diff --git a/spec/lib/gitlab/ci/stage/seed_spec.rb b/spec/lib/gitlab/ci/stage/seed_spec.rb
new file mode 100644
index 00000000000..15bcce04447
--- /dev/null
+++ b/spec/lib/gitlab/ci/stage/seed_spec.rb
@@ -0,0 +1,54 @@
+require 'spec_helper'
+
+describe Gitlab::Ci::Stage::Seed do
+  let(:pipeline) { create(:ci_empty_pipeline) }
+
+  let(:builds) do
+    [{ name: 'rspec' }, { name: 'spinach' }]
+  end
+
+  subject do
+    described_class.new(pipeline, 'test', builds)
+  end
+
+  describe '#stage' do
+    it 'returns hash attributes of a stage' do
+      expect(subject.stage).to be_a Hash
+      expect(subject.stage).to include(:name, :project)
+    end
+  end
+
+  describe '#builds' do
+    it 'returns hash attributes of all builds' do
+      expect(subject.builds.size).to eq 2
+      expect(subject.builds).to all(include(pipeline: pipeline))
+      expect(subject.builds).to all(include(project: pipeline.project))
+      expect(subject.builds).to all(include(ref: 'master'))
+      expect(subject.builds).to all(include(tag: false))
+      expect(subject.builds)
+        .to all(include(trigger_request: pipeline.trigger_requests.first))
+    end
+  end
+
+  describe '#user=' do
+    let(:user) { create(:user) }
+
+    it 'assignes relevant pipeline attributes' do
+      subject.user = user
+
+      expect(subject.builds).to all(include(user: user))
+    end
+  end
+
+  describe '#create!' do
+    it 'creates all stages and builds' do
+      subject.create!
+
+      expect(pipeline.reload.stages.count).to eq 1
+      expect(pipeline.reload.builds.count).to eq 2
+      expect(pipeline.builds).to all(satisfy { |job| job.stage_id.present? })
+      expect(pipeline.builds).to all(satisfy { |job| job.pipeline.present? })
+      expect(pipeline.builds).to all(satisfy { |job| job.project.present? })
+    end
+  end
+end
diff --git a/spec/lib/gitlab/ci/stage/seeds_spec.rb b/spec/lib/gitlab/ci/stage/seeds_spec.rb
deleted file mode 100644
index 3824a868fb2..00000000000
--- a/spec/lib/gitlab/ci/stage/seeds_spec.rb
+++ /dev/null
@@ -1,66 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::Ci::Stage::Seeds do
-  before do
-    subject.append_stage('test', [{ name: 'rspec' }, { name: 'spinach' }])
-    subject.append_stage('deploy', [{ name: 'prod', script: 'cap deploy' }])
-  end
-
-  describe '#has_stages?' do
-    it { is_expected.to have_stages }
-  end
-
-  describe '#stages' do
-    it 'returns hashes of all stages' do
-      expect(subject.stages.size).to eq 2
-      expect(subject.stages).to all(be_a Hash)
-    end
-  end
-
-  describe '#jobs' do
-    it 'returns all jobs in all stages' do
-      expect(subject.jobs.size).to eq 3
-    end
-  end
-
-  describe '#pipeline=' do
-    let(:pipeline) do
-      create(:ci_empty_pipeline, ref: 'feature', tag: true)
-    end
-
-    it 'assignes relevant pipeline attributes' do
-      trigger_request = pipeline.trigger_requests.first
-
-      subject.pipeline = pipeline
-
-      expect(subject.stages).to all(include(pipeline: pipeline))
-      expect(subject.stages).to all(include(project: pipeline.project))
-      expect(subject.jobs).to all(include(pipeline: pipeline))
-      expect(subject.jobs).to all(include(project: pipeline.project))
-      expect(subject.jobs).to all(include(ref: 'feature'))
-      expect(subject.jobs).to all(include(tag: true))
-      expect(subject.jobs).to all(include(trigger_request: trigger_request))
-    end
-  end
-
-  describe '#user=' do
-    let(:user) { create(:user) }
-
-    it 'assignes relevant pipeline attributes' do
-      subject.user = user
-
-      expect(subject.jobs).to all(include(user: user))
-    end
-  end
-
-  describe '#to_attributes' do
-    it 'exposes stage attributes with nested jobs' do
-      attributes = [{ name: 'test', builds_attributes:
-                      [{ name: 'rspec' }, { name: 'spinach' }] },
-                    { name: 'deploy', builds_attributes:
-                      [{ name: 'prod', script: 'cap deploy' }] }]
-
-      expect(subject.to_attributes).to eq attributes
-    end
-  end
-end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 17e10a5322e..63dbf1e9d8b 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -208,8 +208,8 @@ describe Ci::Pipeline, models: true do
       end
 
       it 'returns preseeded stage seeds object' do
-        expect(pipeline.stage_seeds).to be_a Gitlab::Ci::Stage::Seeds
-        expect(pipeline.stage_seeds.stages).to all(include(pipeline: pipeline))
+        expect(pipeline.stage_seeds).to all(be_a Gitlab::Ci::Stage::Seed)
+        expect(pipeline.stage_seeds.count).to eq 1
       end
     end
 
@@ -513,17 +513,17 @@ describe Ci::Pipeline, models: true do
     end
   end
 
-  describe '#has_stages?' do
-    context 'when pipeline has stages' do
+  describe '#has_stage_seedss?' do
+    context 'when pipeline has stage seeds' do
       subject { create(:ci_pipeline_with_one_job) }
 
-      it { is_expected.to have_stages }
+      it { is_expected.to have_stage_seeds }
     end
 
-    context 'when pipeline does not have stages' do
+    context 'when pipeline does not have stage seeds' do
       subject { create(:ci_pipeline_without_jobs) }
 
-      it { is_expected.not_to have_stages }
+      it { is_expected.not_to have_stage_seeds }
     end
   end