Merge branch 'fix-retries-on-old-builds' into 'master'

Use value of `yaml_variables` and `when` from config_processor if undefined

## What does this MR do?
Uses `yaml_variables` and `when` from config_processor if values for these are undefined.

## Why was this MR needed?
Old builds doesn't have this columns initialised. 
Thus makes the retries on these builds to not work properly.
This is regression introduced by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5296.

## Does this MR meet the acceptance criteria?

- Tests
  - [ ] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)


See merge request !5342

app/models/ci/build.rb

@@ -145,12 +145,7 @@ module Ci
     end
 
     def variables
-      variables = []
-      variables += predefined_variables
-      variables += yaml_variables if yaml_variables
-      variables += project_variables
-      variables += trigger_variables
-      variables
+      predefined_variables + yaml_variables + project_variables + trigger_variables
     end
 
     def merge_request
@@ -409,6 +404,14 @@ module Ci
       self.update(artifacts_expire_at: nil)
     end
 
+    def when
+      read_attribute(:when) || build_attributes_from_config[:when] || 'on_success'
+    end
+
+    def yaml_variables
+      read_attribute(:yaml_variables) || build_attributes_from_config[:yaml_variables] || []
+    end
+
     private
 
     def update_artifacts_size
@@ -451,5 +454,11 @@ module Ci
       variables << { key: :CI_BUILD_TRIGGERED, value: 'true', public: true } if trigger_request
       variables
     end
+
+    def build_attributes_from_config
+      return {} unless pipeline.config_processor
+      
+      pipeline.config_processor.build_attributes(name)
+    end
   end
 end

lib/ci/gitlab_ci_yaml_processor.rb

@@ -44,23 +44,51 @@ module Ci
     end
 
     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)
+      jobs_for_ref(ref, tag, trigger_request).map do |name, _|
+        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|

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'

spec/lib/gitlab/badge/build_spec.rb

@@ -113,7 +113,7 @@ describe Gitlab::Badge::Build do
                                     sha: sha,
                                     ref: branch)
 
-    create(:ci_build, pipeline: pipeline)
+    create(:ci_build, pipeline: pipeline, stage: 'notify')
   end
 
   def status_node(data, status)

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
@@ -721,4 +769,69 @@ 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
+
+  describe '#retryable?' do
+    context 'when build is running' do
+      before { build.run! }
+
+      it 'should return false' do
+        expect(build.retryable?).to be false
+      end
+    end
+
+    context 'when build is finished' do
+      before { build.success! }
+
+      it 'should return true' do
+        expect(build.retryable?).to be true
+      end
+    end
+  end
 end