Commit 6593e3ce authored by Rémy Coutable's avatar Rémy Coutable

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
parents e57c0c89 6b07a5b2
...@@ -145,12 +145,7 @@ module Ci ...@@ -145,12 +145,7 @@ module Ci
end end
def variables def variables
variables = [] predefined_variables + yaml_variables + project_variables + trigger_variables
variables += predefined_variables
variables += yaml_variables if yaml_variables
variables += project_variables
variables += trigger_variables
variables
end end
def merge_request def merge_request
...@@ -409,6 +404,14 @@ module Ci ...@@ -409,6 +404,14 @@ module Ci
self.update(artifacts_expire_at: nil) self.update(artifacts_expire_at: nil)
end 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 private
def update_artifacts_size def update_artifacts_size
...@@ -451,5 +454,11 @@ module Ci ...@@ -451,5 +454,11 @@ module Ci
variables << { key: :CI_BUILD_TRIGGERED, value: 'true', public: true } if trigger_request variables << { key: :CI_BUILD_TRIGGERED, value: 'true', public: true } if trigger_request
variables variables
end end
def build_attributes_from_config
return {} unless pipeline.config_processor
pipeline.config_processor.build_attributes(name)
end
end end
end end
...@@ -44,23 +44,51 @@ module Ci ...@@ -44,23 +44,51 @@ module Ci
end end
def builds_for_ref(ref, tag = false, trigger_request = nil) def builds_for_ref(ref, tag = false, trigger_request = nil)
jobs_for_ref(ref, tag, trigger_request).map do |name, job| jobs_for_ref(ref, tag, trigger_request).map do |name, _|
build_job(name, job) build_attributes(name)
end end
end end
def builds_for_stage_and_ref(stage, ref, tag = false, trigger_request = nil) 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| jobs_for_stage_and_ref(stage, ref, tag, trigger_request).map do |name, _|
build_job(name, job) build_attributes(name)
end end
end end
def builds def builds
@jobs.map do |name, job| @jobs.map do |name, _|
build_job(name, job) build_attributes(name)
end end
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 private
def initial_parsing def initial_parsing
...@@ -89,33 +117,6 @@ module Ci ...@@ -89,33 +117,6 @@ module Ci
@jobs[name] = { stage: stage }.merge(job) @jobs[name] = { stage: stage }.merge(job)
end 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) def yaml_variables(name)
variables = global_variables.merge(job_variables(name)) variables = global_variables.merge(job_variables(name))
variables.map do |key, value| variables.map do |key, value|
......
...@@ -3,6 +3,8 @@ include ActionDispatch::TestProcess ...@@ -3,6 +3,8 @@ include ActionDispatch::TestProcess
FactoryGirl.define do FactoryGirl.define do
factory :ci_build, class: Ci::Build do factory :ci_build, class: Ci::Build do
name 'test' name 'test'
stage 'test'
stage_idx 0
ref 'master' ref 'master'
tag false tag false
created_at 'Di 29. Okt 09:50:00 CET 2013' created_at 'Di 29. Okt 09:50:00 CET 2013'
......
...@@ -113,7 +113,7 @@ describe Gitlab::Badge::Build do ...@@ -113,7 +113,7 @@ describe Gitlab::Badge::Build do
sha: sha, sha: sha,
ref: branch) ref: branch)
create(:ci_build, pipeline: pipeline) create(:ci_build, pipeline: pipeline, stage: 'notify')
end end
def status_node(data, status) def status_node(data, status)
......
...@@ -191,16 +191,16 @@ describe Ci::Build, models: true do ...@@ -191,16 +191,16 @@ describe Ci::Build, models: true do
end end
describe '#variables' do describe '#variables' do
context 'returns variables' do let(:predefined_variables) do
subject { build.variables } [
{ key: :CI_BUILD_NAME, value: 'test', public: true },
{ key: :CI_BUILD_STAGE, value: 'test', public: true },
]
end
let(:predefined_variables) do subject { build.variables }
[
{ key: :CI_BUILD_NAME, value: 'test', public: true },
{ key: :CI_BUILD_STAGE, value: 'stage', public: true },
]
end
context 'returns variables' do
let(:yaml_variables) do let(:yaml_variables) do
[ [
{ key: :DB_NAME, value: 'postgres', public: true } { key: :DB_NAME, value: 'postgres', public: true }
...@@ -208,7 +208,7 @@ describe Ci::Build, models: true do ...@@ -208,7 +208,7 @@ describe Ci::Build, models: true do
end end
before do before do
build.update_attributes(stage: 'stage', yaml_variables: yaml_variables) build.yaml_variables = yaml_variables
end end
it { is_expected.to eq(predefined_variables + yaml_variables) } it { is_expected.to eq(predefined_variables + yaml_variables) }
...@@ -262,6 +262,54 @@ describe Ci::Build, models: true do ...@@ -262,6 +262,54 @@ describe Ci::Build, models: true do
end end
end 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 end
describe '#has_tags?' do describe '#has_tags?' do
...@@ -721,4 +769,69 @@ describe Ci::Build, models: true do ...@@ -721,4 +769,69 @@ describe Ci::Build, models: true do
end end
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment