Commit cf0283c8 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'fix/require-build-script-configuration-entry' into 'master'

Make job script a required configuration entry

## What does this MR do?

This MR makes a job script a required configuration entry.

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing

## What are the relevant issue numbers?

Closes #24575

See merge request !7566
parents 7ef7135a 7023e766
---
title: Make job script a required configuration entry
merge_request: 7566
author:
...@@ -4,12 +4,6 @@ module Gitlab ...@@ -4,12 +4,6 @@ module Gitlab
# Base GitLab CI Configuration facade # Base GitLab CI Configuration facade
# #
class Config class Config
##
# Temporary delegations that should be removed after refactoring
#
delegate :before_script, :image, :services, :after_script, :variables,
:stages, :cache, :jobs, to: :@global
def initialize(config) def initialize(config)
@config = Loader.new(config).load! @config = Loader.new(config).load!
...@@ -28,6 +22,41 @@ module Gitlab ...@@ -28,6 +22,41 @@ module Gitlab
def to_hash def to_hash
@config @config
end end
##
# Temporary method that should be removed after refactoring
#
def before_script
@global.before_script_value
end
def image
@global.image_value
end
def services
@global.services_value
end
def after_script
@global.after_script_value
end
def variables
@global.variables_value
end
def stages
@global.stages_value
end
def cache
@global.cache_value
end
def jobs
@global.jobs_value
end
end end
end end
end end
...@@ -66,8 +66,6 @@ module Gitlab ...@@ -66,8 +66,6 @@ module Gitlab
@entries[symbol].value @entries[symbol].value
end end
alias_method symbol.to_sym, "#{symbol}_value".to_sym
end end
end end
end end
......
...@@ -13,12 +13,10 @@ module Gitlab ...@@ -13,12 +13,10 @@ module Gitlab
type stage when artifacts cache dependencies before_script type stage when artifacts cache dependencies before_script
after_script variables environment] after_script variables environment]
attributes :tags, :allow_failure, :when, :dependencies
validations do validations do
validates :config, allowed_keys: ALLOWED_KEYS validates :config, allowed_keys: ALLOWED_KEYS
validates :config, presence: true validates :config, presence: true
validates :script, presence: true
validates :name, presence: true validates :name, presence: true
validates :name, type: Symbol validates :name, type: Symbol
...@@ -77,6 +75,8 @@ module Gitlab ...@@ -77,6 +75,8 @@ module Gitlab
:cache, :image, :services, :only, :except, :variables, :cache, :image, :services, :only, :except, :variables,
:artifacts, :commands, :environment :artifacts, :commands, :environment
attributes :script, :tags, :allow_failure, :when, :dependencies
def compose!(deps = nil) def compose!(deps = nil)
super do super do
if type_defined? && !stage_defined? if type_defined? && !stage_defined?
...@@ -118,20 +118,20 @@ module Gitlab ...@@ -118,20 +118,20 @@ module Gitlab
def to_hash def to_hash
{ name: name, { name: name,
before_script: before_script, before_script: before_script_value,
script: script, script: script_value,
commands: commands, commands: commands,
image: image, image: image_value,
services: services, services: services_value,
stage: stage, stage: stage_value,
cache: cache, cache: cache_value,
only: only, only: only_value,
except: except, except: except_value,
variables: variables_defined? ? variables : nil, variables: variables_defined? ? variables_value : nil,
environment: environment_defined? ? environment : nil, environment: environment_defined? ? environment_value : nil,
environment_name: environment_defined? ? environment[:name] : nil, environment_name: environment_defined? ? environment_value[:name] : nil,
artifacts: artifacts, artifacts: artifacts_value,
after_script: after_script } after_script: after_script_value }
end end
end end
end end
......
...@@ -1124,8 +1124,8 @@ EOT ...@@ -1124,8 +1124,8 @@ EOT
end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra config should be a hash") end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra config should be a hash")
end end
it "returns errors if there are unknown parameters that are hashes, but doesn't have a script" do it "returns errors if services configuration is not correct" do
config = YAML.dump({ extra: { services: "test" } }) config = YAML.dump({ extra: { script: 'rspec', services: "test" } })
expect do expect do
GitlabCiYamlProcessor.new(config, path) GitlabCiYamlProcessor.new(config, path)
end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra:services config should be an array of strings") end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra:services config should be an array of strings")
......
...@@ -60,9 +60,9 @@ describe Gitlab::Ci::Config::Entry::Global do ...@@ -60,9 +60,9 @@ describe Gitlab::Ci::Config::Entry::Global do
end end
context 'when not composed' do context 'when not composed' do
describe '#before_script' do describe '#before_script_value' do
it 'returns nil' do it 'returns nil' do
expect(global.before_script).to be nil expect(global.before_script_value).to be nil
end end
end end
...@@ -82,40 +82,40 @@ describe Gitlab::Ci::Config::Entry::Global do ...@@ -82,40 +82,40 @@ describe Gitlab::Ci::Config::Entry::Global do
end end
end end
describe '#before_script' do describe '#before_script_value' do
it 'returns correct script' do it 'returns correct script' do
expect(global.before_script).to eq ['ls', 'pwd'] expect(global.before_script_value).to eq ['ls', 'pwd']
end end
end end
describe '#image' do describe '#image_value' do
it 'returns valid image' do it 'returns valid image' do
expect(global.image).to eq 'ruby:2.2' expect(global.image_value).to eq 'ruby:2.2'
end end
end end
describe '#services' do describe '#services_value' do
it 'returns array of services' do it 'returns array of services' do
expect(global.services).to eq ['postgres:9.1', 'mysql:5.5'] expect(global.services_value).to eq ['postgres:9.1', 'mysql:5.5']
end end
end end
describe '#after_script' do describe '#after_script_value' do
it 'returns after script' do it 'returns after script' do
expect(global.after_script).to eq ['make clean'] expect(global.after_script_value).to eq ['make clean']
end end
end end
describe '#variables' do describe '#variables_value' do
it 'returns variables' do it 'returns variables' do
expect(global.variables).to eq(VAR: 'value') expect(global.variables_value).to eq(VAR: 'value')
end end
end end
describe '#stages' do describe '#stages_value' do
context 'when stages key defined' do context 'when stages key defined' do
it 'returns array of stages' do it 'returns array of stages' do
expect(global.stages).to eq %w[build pages] expect(global.stages_value).to eq %w[build pages]
end end
end end
...@@ -126,21 +126,21 @@ describe Gitlab::Ci::Config::Entry::Global do ...@@ -126,21 +126,21 @@ describe Gitlab::Ci::Config::Entry::Global do
end end
it 'returns array of types as stages' do it 'returns array of types as stages' do
expect(global.stages).to eq %w[test deploy] expect(global.stages_value).to eq %w[test deploy]
end end
end end
end end
describe '#cache' do describe '#cache_value' do
it 'returns cache configuration' do it 'returns cache configuration' do
expect(global.cache) expect(global.cache_value)
.to eq(key: 'k', untracked: true, paths: ['public/']) .to eq(key: 'k', untracked: true, paths: ['public/'])
end end
end end
describe '#jobs' do describe '#jobs_value' do
it 'returns jobs configuration' do it 'returns jobs configuration' do
expect(global.jobs).to eq( expect(global.jobs_value).to eq(
rspec: { name: :rspec, rspec: { name: :rspec,
script: %w[rspec ls], script: %w[rspec ls],
before_script: ['ls', 'pwd'], before_script: ['ls', 'pwd'],
...@@ -185,21 +185,21 @@ describe Gitlab::Ci::Config::Entry::Global do ...@@ -185,21 +185,21 @@ describe Gitlab::Ci::Config::Entry::Global do
end end
end end
describe '#variables' do describe '#variables_value' do
it 'returns default value for variables' do it 'returns default value for variables' do
expect(global.variables).to eq({}) expect(global.variables_value).to eq({})
end end
end end
describe '#stages' do describe '#stages_value' do
it 'returns an array of default stages' do it 'returns an array of default stages' do
expect(global.stages).to eq %w[build test deploy] expect(global.stages_value).to eq %w[build test deploy]
end end
end end
describe '#cache' do describe '#cache_value' do
it 'returns correct cache definition' do it 'returns correct cache definition' do
expect(global.cache).to eq(key: 'a') expect(global.cache_value).to eq(key: 'a')
end end
end end
end end
...@@ -217,9 +217,9 @@ describe Gitlab::Ci::Config::Entry::Global do ...@@ -217,9 +217,9 @@ describe Gitlab::Ci::Config::Entry::Global do
{ variables: nil, rspec: { script: 'rspec' } } { variables: nil, rspec: { script: 'rspec' } }
end end
describe '#variables' do describe '#variables_value' do
it 'undefined entry returns a default value' do it 'undefined entry returns a default value' do
expect(global.variables).to eq({}) expect(global.variables_value).to eq({})
end end
end end
end end
...@@ -245,9 +245,9 @@ describe Gitlab::Ci::Config::Entry::Global do ...@@ -245,9 +245,9 @@ describe Gitlab::Ci::Config::Entry::Global do
end end
end end
describe '#before_script' do describe '#before_script_value' do
it 'returns nil' do it 'returns nil' do
expect(global.before_script).to be_nil expect(global.before_script_value).to be_nil
end end
end end
end end
......
...@@ -19,8 +19,7 @@ describe Gitlab::Ci::Config::Entry::Job do ...@@ -19,8 +19,7 @@ describe Gitlab::Ci::Config::Entry::Job do
let(:entry) { described_class.new(config, name: ''.to_sym) } let(:entry) { described_class.new(config, name: ''.to_sym) }
it 'reports error' do it 'reports error' do
expect(entry.errors) expect(entry.errors).to include "job name can't be blank"
.to include "job name can't be blank"
end end
end end
end end
...@@ -56,6 +55,15 @@ describe Gitlab::Ci::Config::Entry::Job do ...@@ -56,6 +55,15 @@ describe Gitlab::Ci::Config::Entry::Job do
end end
end end
end end
context 'when script is not provided' do
let(:config) { { stage: 'test' } }
it 'returns error about missing script entry' do
expect(entry).not_to be_valid
expect(entry.errors).to include "job script can't be blank"
end
end
end end
end end
...@@ -78,7 +86,7 @@ describe Gitlab::Ci::Config::Entry::Job do ...@@ -78,7 +86,7 @@ describe Gitlab::Ci::Config::Entry::Job do
before { entry.compose!(deps) } before { entry.compose!(deps) }
let(:config) do let(:config) do
{ image: 'some_image', cache: { key: 'test' } } { script: 'rspec', image: 'some_image', cache: { key: 'test' } }
end end
it 'overrides global config' do it 'overrides global config' do
......
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