Use value of `yaml_variables` and `when` from config_processor if undefinedfix-retries-on-old-builds

4 files changed, 153 insertions, 42 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index ffac3a22efc..fc71f6cb3f8 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -123,7 +123,7 @@ module Ci
     def variables
       variables = []
       variables += predefined_variables
-      variables += yaml_variables if yaml_variables
+      variables += yaml_variables
       variables += project_variables
       variables += trigger_variables
       variables
@@ -385,6 +385,14 @@ module Ci
       self.update(artifacts_expire_at: nil)
     end
 
+    def when
+      read_attribute(:when) || build_attributes[:when] || 'on_success'
+    end
+
+    def yaml_variables
+      read_attribute(:yaml_variables) || build_attributes[:yaml_variables] || []
+    end
+
     private
 
     def update_artifacts_size
@@ -427,5 +435,10 @@ module Ci
       variables << { key: :CI_BUILD_TRIGGERED, value: 'true', public: true } if trigger_request
       variables
     end
+
+    def build_attributes
+      return {} unless pipeline.config_processor
+      pipeline.config_processor.build_attributes(name)
+    end
   end
 end
diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb
index a48dc542b14..a10d6331143 100644
--- a/lib/ci/gitlab_ci_yaml_processor.rb
+++ b/lib/ci/gitlab_ci_yaml_processor.rb
@@ -45,22 +45,50 @@ module Ci
 
     def builds_for_ref(ref, tag = false, trigger_request = nil)
       jobs_for_ref(ref, tag, trigger_request).map do |name, job|
-        build_job(name, job)
+        build_attributes(name, _)
       end
     end
 
     def builds_for_stage_and_ref(stage, ref, tag = false, trigger_request = nil)
-      jobs_for_stage_and_ref(stage, ref, tag, trigger_request).map do |name, job|
-        build_job(name, job)
+      jobs_for_stage_and_ref(stage, ref, tag, trigger_request).map do |name, _|
+        build_attributes(name)
       end
     end
 
     def builds
-      @jobs.map do |name, job|
-        build_job(name, job)
+      @jobs.map do |name, _|
+        build_attributes(name)
       end
     end
 
+    def build_attributes(name)
+      job = @jobs[name.to_sym] || {}
+      {
+        stage_idx: @stages.index(job[:stage]),
+        stage: job[:stage],
+        ##
+        # Refactoring note:
+        #  - before script behaves differently than after script
+        #  - after script returns an array of commands
+        #  - before script should be a concatenated command
+        commands: [job[:before_script] || @before_script, job[:script]].flatten.compact.join("\n"),
+        tag_list: job[:tags] || [],
+        name: name,
+        allow_failure: job[:allow_failure] || false,
+        when: job[:when] || 'on_success',
+        environment: job[:environment],
+        yaml_variables: yaml_variables(name),
+        options: {
+          image: job[:image] || @image,
+          services: job[:services] || @services,
+          artifacts: job[:artifacts],
+          cache: job[:cache] || @cache,
+          dependencies: job[:dependencies],
+          after_script: job[:after_script] || @after_script,
+        }.compact
+      }
+    end
+
     private
 
     def initial_parsing
@@ -89,33 +117,6 @@ module Ci
       @jobs[name] = { stage: stage }.merge(job)
     end
 
-    def build_job(name, job)
-      {
-        stage_idx: @stages.index(job[:stage]),
-        stage: job[:stage],
-        ##
-        # Refactoring note:
-        #  - before script behaves differently than after script
-        #  - after script returns an array of commands
-        #  - before script should be a concatenated command
-        commands: [job[:before_script] || @before_script, job[:script]].flatten.compact.join("\n"),
-        tag_list: job[:tags] || [],
-        name: name,
-        allow_failure: job[:allow_failure] || false,
-        when: job[:when] || 'on_success',
-        environment: job[:environment],
-        yaml_variables: yaml_variables(name),
-        options: {
-          image: job[:image] || @image,
-          services: job[:services] || @services,
-          artifacts: job[:artifacts],
-          cache: job[:cache] || @cache,
-          dependencies: job[:dependencies],
-          after_script: job[:after_script] || @after_script,
-        }.compact
-      }
-    end
-
     def yaml_variables(name)
       variables = global_variables.merge(job_variables(name))
       variables.map do |key, value|
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index 5fb671df570..16caa5f6888 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -3,6 +3,8 @@ include ActionDispatch::TestProcess
 FactoryGirl.define do
   factory :ci_build, class: Ci::Build do
     name 'test'
+    stage 'test'
+    stage_idx 0
     ref 'master'
     tag false
     created_at 'Di 29. Okt 09:50:00 CET 2013'
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 481416319dd..dc4a7075a80 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -191,16 +191,16 @@ describe Ci::Build, models: true do
   end
 
   describe '#variables' do
-    context 'returns variables' do
-      subject { build.variables }
+    let(:predefined_variables) do
+      [
+        { key: :CI_BUILD_NAME, value: 'test', public: true },
+        { key: :CI_BUILD_STAGE, value: 'test', public: true },
+      ]
+    end
 
-      let(:predefined_variables) do
-        [
-          { key: :CI_BUILD_NAME, value: 'test', public: true },
-          { key: :CI_BUILD_STAGE, value: 'stage', public: true },
-        ]
-      end
+    subject { build.variables }
 
+    context 'returns variables' do
       let(:yaml_variables) do
         [
           { key: :DB_NAME, value: 'postgres', public: true }
@@ -208,7 +208,7 @@ describe Ci::Build, models: true do
       end
 
       before do
-        build.update_attributes(stage: 'stage', yaml_variables: yaml_variables)
+        build.yaml_variables = yaml_variables
       end
 
       it { is_expected.to eq(predefined_variables + yaml_variables) }
@@ -262,6 +262,54 @@ describe Ci::Build, models: true do
         end
       end
     end
+
+    context 'when yaml_variables is undefined' do
+      before do
+        build.yaml_variables = nil
+      end
+
+      context 'use from gitlab-ci.yml' do
+        before do
+          stub_ci_pipeline_yaml_file(config)
+        end
+
+        context 'if config is not found' do
+          let(:config) { nil }
+
+          it { is_expected.to eq(predefined_variables) }
+        end
+
+        context 'if config does not have a questioned job' do
+          let(:config) do
+            YAML.dump({
+                        test_other: {
+                          script: 'Hello World'
+                        }
+                      })
+          end
+
+          it { is_expected.to eq(predefined_variables) }
+        end
+
+        context 'if config has variables' do
+          let(:config) do
+            YAML.dump({
+                        test: {
+                          script: 'Hello World',
+                          variables: {
+                            KEY: 'value'
+                          }
+                        }
+                      })
+          end
+          let(:variables) do
+            [{ key: :KEY, value: 'value', public: true }]
+          end
+
+          it { is_expected.to eq(predefined_variables + variables) }
+        end
+      end
+    end
   end
 
   describe '#has_tags?' do
@@ -670,4 +718,51 @@ describe Ci::Build, models: true do
       end
     end
   end
+
+  describe '#when' do
+    subject { build.when }
+
+    context 'if is undefined' do
+      before do
+        build.when = nil
+      end
+
+      context 'use from gitlab-ci.yml' do
+        before do
+          stub_ci_pipeline_yaml_file(config)
+        end
+
+        context 'if config is not found' do
+          let(:config) { nil }
+
+          it { is_expected.to eq('on_success') }
+        end
+
+        context 'if config does not have a questioned job' do
+          let(:config) do
+            YAML.dump({
+              test_other: {
+                script: 'Hello World'
+              }
+            })
+          end
+
+          it { is_expected.to eq('on_success') }
+        end
+
+        context 'if config has when' do
+          let(:config) do
+            YAML.dump({
+              test: {
+                script: 'Hello World',
+                when: 'always'
+              }
+            })
+          end
+
+          it { is_expected.to eq('always') }
+        end
+      end
+    end
+  end
 end