diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 46dad59eb8c..94b2fbc082b 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -1,15 +1,17 @@ module Gitlab module Ci - ## + # # Base GitLab CI Configuration facade # class Config ConfigError = Class.new(StandardError) def initialize(config, opts = {}) - @config = Config::Extendable + initial_config = Config::Extendable .new(build_config(config, opts)) .to_hash + processor = ::Gitlab::Ci::ExternalFiles::Processor.new(initial_config) + @config = processor.perform @global = Entry::Global.new(@config) @global.compose! diff --git a/lib/gitlab/ci/config/entry/global.rb b/lib/gitlab/ci/config/entry/global.rb index 401e566de77..a4ec8f0ff2f 100644 --- a/lib/gitlab/ci/config/entry/global.rb +++ b/lib/gitlab/ci/config/entry/global.rb @@ -33,15 +33,11 @@ module Gitlab entry :cache, Entry::Cache, description: 'Configure caching between build jobs.' - entry :includes, Entry::Includes, - description: 'External GitlLab Ci files' - helpers :before_script, :image, :services, :after_script, - :variables, :stages, :types, :cache, :jobs, :includes + :variables, :stages, :types, :cache, :jobs def compose!(_deps = nil) super(self) do - append_external_files! compose_jobs! compose_deprecated_entries! end @@ -49,10 +45,6 @@ module Gitlab private - def append_external_files! - return if includes_value.nil? - end - def compose_jobs! factory = Entry::Factory.new(Entry::Jobs) .value(@config.except(*self.class.nodes.keys)) diff --git a/lib/gitlab/ci/config/entry/includes.rb b/lib/gitlab/ci/config/entry/includes.rb deleted file mode 100644 index c7a8c7960e8..00000000000 --- a/lib/gitlab/ci/config/entry/includes.rb +++ /dev/null @@ -1,22 +0,0 @@ -module Gitlab - module Ci - class Config - module Entry - ## - # Entry that represents a Docker image. - # - class Includes < Node - include Validatable - - validations do - validates :config, array_or_string: true, external_file: true, allow_nil: true - end - - def value - Array(@config) - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb index 5b4d5d53821..b3c889ee92f 100644 --- a/lib/gitlab/ci/config/entry/validators.rb +++ b/lib/gitlab/ci/config/entry/validators.rb @@ -60,38 +60,6 @@ module Gitlab end end - class ArrayOrStringValidator < ActiveModel::EachValidator - def validate_each(record, attribute, value) - unless value.is_a?(Array) || value.is_a?(String) - record.errors.add(attribute, 'should be an array or a string') - end - end - end - - class ExternalFileValidator < ActiveModel::EachValidator - def validate_each(record, attribute, value) - if value.is_a?(Array) - value.each do |path| - validate_external_file(path, record, attribute) - end - else - validate_external_file(value, record, attribute) - end - end - - private - - def validate_external_file(value, record, attribute) - unless valid_url?(value) - record.errors.add(attribute, 'should be a valid local or remote file') - end - end - - def valid_url?(value) - Gitlab::UrlSanitizer.valid?(value) || File.exists?("#{Rails.root}/#{value}") - end - end - class KeyValidator < ActiveModel::EachValidator include LegacyValidationHelpers diff --git a/lib/gitlab/ci/external_files/external_file.rb b/lib/gitlab/ci/external_files/external_file.rb index 9bad8231c42..47d6d1c0fdc 100644 --- a/lib/gitlab/ci/external_files/external_file.rb +++ b/lib/gitlab/ci/external_files/external_file.rb @@ -4,7 +4,6 @@ module Gitlab module Ci module ExternalFiles class ExternalFile - def initialize(value) @value = value end @@ -18,7 +17,7 @@ module Gitlab end def valid? - remote_url? || File.exists?(base_path) + remote_url? || File.exist?(base_path) end private diff --git a/lib/gitlab/ci/external_files/mapper.rb b/lib/gitlab/ci/external_files/mapper.rb index 5deb2f5a2e5..4b5d2ddd6a1 100644 --- a/lib/gitlab/ci/external_files/mapper.rb +++ b/lib/gitlab/ci/external_files/mapper.rb @@ -2,14 +2,11 @@ module Gitlab module Ci module ExternalFiles class Mapper - def self.fetch_paths(values) paths = values.fetch(:includes, []) normalize_paths(paths) end - private - def self.normalize_paths(paths) if paths.is_a?(String) [build_external_file(paths)] diff --git a/lib/gitlab/ci/external_files/processor.rb b/lib/gitlab/ci/external_files/processor.rb index f38bc7633b5..221b6f58b98 100644 --- a/lib/gitlab/ci/external_files/processor.rb +++ b/lib/gitlab/ci/external_files/processor.rb @@ -25,7 +25,7 @@ module Gitlab attr_reader :values, :external_files def validate_external_file(external_file) - unless external_file.valid? + unless external_file.valid? raise ExternalFileError, 'External files should be a valid local or remote file' end end @@ -39,7 +39,6 @@ module Gitlab values.delete(:includes) values end - end end end diff --git a/spec/lib/gitlab/ci/config/entry/global_spec.rb b/spec/lib/gitlab/ci/config/entry/global_spec.rb index ff623b95be8..1860ed79bfd 100644 --- a/spec/lib/gitlab/ci/config/entry/global_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/global_spec.rb @@ -13,7 +13,7 @@ describe Gitlab::Ci::Config::Entry::Global do expect(described_class.nodes.keys) .to match_array(%i[before_script image services after_script variables stages - types cache includes]) + types cache]) end end end @@ -42,7 +42,7 @@ describe Gitlab::Ci::Config::Entry::Global do end it 'creates node object for each entry' do - expect(global.descendants.count).to eq 9 + expect(global.descendants.count).to eq 8 end it 'creates node object using valid class' do @@ -189,7 +189,7 @@ describe Gitlab::Ci::Config::Entry::Global do describe '#nodes' do it 'instantizes all nodes' do - expect(global.descendants.count).to eq 9 + expect(global.descendants.count).to eq 8 end it 'contains unspecified nodes' do diff --git a/spec/lib/gitlab/ci/config/entry/includes_spec.rb b/spec/lib/gitlab/ci/config/entry/includes_spec.rb deleted file mode 100644 index d72503535ed..00000000000 --- a/spec/lib/gitlab/ci/config/entry/includes_spec.rb +++ /dev/null @@ -1,97 +0,0 @@ -require 'rails_helper' - -describe Gitlab::Ci::Config::Entry::Includes do - let(:entry) { described_class.new(config) } - - shared_examples 'valid external file' do - it 'should be valid' do - expect(entry).to be_valid - end - - it 'should not return any error' do - expect(entry.errors).to be_empty - end - end - - shared_examples 'invalid external file' do - it 'should not be valid' do - expect(entry).not_to be_valid - end - - it 'should return an error' do - expect(entry.errors.first).to match(/should be a valid local or remote file/) - end - end - - describe "#valid?" do - context 'with no external file given' do - let(:config) { nil } - - it_behaves_like 'valid external file' - end - - context 'with multiple external files' do - let(:config) { %w(https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-2.yml) } - - it_behaves_like 'valid external file' - end - - context 'with just one external file' do - let(:config) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } - - it_behaves_like 'valid external file' - end - - context 'when they contain valid URLs' do - let(:config) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } - - it_behaves_like 'valid external file' - end - - context 'when they contain valid relative URLs' do - let(:config) { '/vendor/gitlab-ci-yml/Auto-DevOps.gitlab-ci.yml' } - - it_behaves_like 'valid external file' - end - - context 'when they not contain valid URLs' do - let(:config) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } - - it_behaves_like 'invalid external file' - end - - context 'when they not contain valid relative URLs' do - let(:config) { '/vendor/gitlab-ci-yml/non-existent-file.yml' } - - it_behaves_like 'invalid external file' - end - end - - describe "#value" do - context 'with multiple external files' do - let(:config) { %w(https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-2.yml) } - it 'should return an array' do - expect(entry.value).to be_an(Array) - expect(entry.value.count).to eq(2) - end - end - - context 'with just one external file' do - let(:config) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } - - it 'should return an array' do - expect(entry.value).to be_an(Array) - expect(entry.value.count).to eq(1) - end - end - - context 'with no external file given' do - let(:config) { nil } - - it 'should return an empty array' do - expect(entry.value).to be_an(Array) - expect(entry.value).to be_empty - end - end - end -end diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 5a78ce783dd..99896b9be5d 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -124,4 +124,61 @@ describe Gitlab::Ci::Config do end end end + + + context "when yml has valid 'includes' defined" do + let(:yml) do + <<-EOS + includes: + - /spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml + - /spec/fixtures/gitlab/ci/external_files/.gitlab-ci-template-2.yml + - https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml + + image: ruby:2.2 + EOS + end + + before do + allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(yml) + end + + it 'should return a composed hash' do + before_script_values = [ + "apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs", "ruby -v", + "which ruby", + "gem install bundler --no-ri --no-rdoc", + "bundle install --jobs $(nproc) \"${FLAGS[@]}\"" + ] + variables = { + AUTO_DEVOPS_DOMAIN: "domain.example.com", + POSTGRES_USER: "user", + POSTGRES_PASSWORD: "testing-password", + POSTGRES_ENABLED: "true", + POSTGRES_DB: "$CI_ENVIRONMENT_SLUG" + } + composed_hash = { + before_script: before_script_values, + image: "ruby:2.2", + rspec: { script: ["bundle exec rspec"] }, + variables: variables + } + + expect(config.to_hash).to eq(composed_hash) + end + end + + context "when config has invalid 'includes' defined" do + let(:yml) do + <<-EOS + includes: invalid + EOS + end + + it 'raises error' do + expect { config }.to raise_error( + ::Gitlab::Ci::ExternalFiles::Processor::ExternalFileError, + /External files should be a valid local or remote file/ + ) + end + end end diff --git a/spec/lib/gitlab/ci/external_files/external_file_spec.rb b/spec/lib/gitlab/ci/external_files/external_file_spec.rb index 80468f63435..33e5e0b3b77 100644 --- a/spec/lib/gitlab/ci/external_files/external_file_spec.rb +++ b/spec/lib/gitlab/ci/external_files/external_file_spec.rb @@ -13,7 +13,7 @@ describe Gitlab::Ci::ExternalFiles::ExternalFile do end context 'when is not a valid remote url' do - let(:value) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } + let(:value) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' } it 'should return false' do expect(external_file.valid?).to be_falsy @@ -39,7 +39,7 @@ describe Gitlab::Ci::ExternalFiles::ExternalFile do end describe "#content" do - let(:external_file_content) { + let(:external_file_content) do <<-HEREDOC before_script: - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs @@ -48,7 +48,7 @@ describe Gitlab::Ci::ExternalFiles::ExternalFile do - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" HEREDOC - } + end context 'with a local file' do let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' } diff --git a/spec/lib/gitlab/ci/external_files/mapper_spec.rb b/spec/lib/gitlab/ci/external_files/mapper_spec.rb index dfbf8bfa098..57cf5e74cdc 100644 --- a/spec/lib/gitlab/ci/external_files/mapper_spec.rb +++ b/spec/lib/gitlab/ci/external_files/mapper_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe Gitlab::Ci::ExternalFiles::Mapper do describe '.fetch_paths' do context 'when includes is defined as string' do - let(:values) { { includes: '/vendor/gitlab-ci-yml/non-existent-file.yml', image: 'ruby:2.2'} } + let(:values) { { includes: '/vendor/gitlab-ci-yml/non-existent-file.yml', image: 'ruby:2.2' } } it 'returns an array' do expect(described_class.fetch_paths(values)).to be_an(Array) @@ -15,21 +15,19 @@ describe Gitlab::Ci::ExternalFiles::Mapper do end context 'when includes is defined as an array' do - let(:values) { { includes: ['https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', '/vendor/gitlab-ci-yml/template.yml'], image: 'ruby:2.2'} } + let(:values) { { includes: ['https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', '/vendor/gitlab-ci-yml/template.yml'], image: 'ruby:2.2' } } it 'returns an array' do expect(described_class.fetch_paths(values)).to be_an(Array) end it 'returns ExternalFile instances' do paths = described_class.fetch_paths(values) - paths.each do |path| - expect(path).to be_an_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile) - end + expect(paths).to all(be_an_instance_of(::Gitlab::Ci::ExternalFiles::ExternalFile)) end end context 'when includes is not defined' do - let(:values) { { image: 'ruby:2.2'} } + let(:values) { { image: 'ruby:2.2' } } it 'returns an empty array' do expect(described_class.fetch_paths(values)).to be_empty diff --git a/spec/lib/gitlab/ci/external_files/processor_spec.rb b/spec/lib/gitlab/ci/external_files/processor_spec.rb index 51ed14de4cf..7a0374a4bce 100644 --- a/spec/lib/gitlab/ci/external_files/processor_spec.rb +++ b/spec/lib/gitlab/ci/external_files/processor_spec.rb @@ -13,7 +13,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do end context 'when an invalid local file is defined' do - let(:values) { { includes: '/vendor/gitlab-ci-yml/non-existent-file.yml', image: 'ruby:2.2'} } + let(:values) { { includes: '/vendor/gitlab-ci-yml/non-existent-file.yml', image: 'ruby:2.2' } } it 'should raise an error' do expect { processor.perform }.to raise_error(described_class::ExternalFileError) @@ -21,7 +21,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do end context 'when an invalid remote file is defined' do - let(:values) { { includes: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2'} } + let(:values) { { includes: 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } } it 'should raise an error' do expect { processor.perform }.to raise_error(described_class::ExternalFileError) @@ -30,7 +30,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do context 'with a valid remote external file is defined' do let(:values) { { includes: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } } - let(:external_file_content) { + let(:external_file_content) do <<-HEREDOC before_script: - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs @@ -47,7 +47,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do script: - bundle exec rubocop HEREDOC - } + end before do allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(external_file_content) @@ -55,7 +55,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do it 'should append the file to the values' do output = processor.perform - expect(output.keys).to match_array([:image, :before_script, :rspec, :rubocop]) + expect(output.keys).to match_array([:image, :before_script, :rspec, :rubocop]) end it "should remove the 'includes' keyword" do @@ -64,8 +64,8 @@ describe Gitlab::Ci::ExternalFiles::Processor do end context 'with a valid local external file is defined' do - let(:values) { { includes: '/vendor/gitlab-ci-yml/template.yml' , image: 'ruby:2.2'} } - let(:external_file_content) { + let(:values) { { includes: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } } + let(:external_file_content) do <<-HEREDOC before_script: - apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs @@ -74,7 +74,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do - gem install bundler --no-ri --no-rdoc - bundle install --jobs $(nproc) "${FLAGS[@]}" HEREDOC - } + end before do allow(File).to receive(:exists?).and_return(true) @@ -83,7 +83,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do it 'should append the file to the values' do output = processor.perform - expect(output.keys).to match_array([:image, :before_script]) + expect(output.keys).to match_array([:image, :before_script]) end it "should remove the 'includes' keyword" do @@ -92,23 +92,23 @@ describe Gitlab::Ci::ExternalFiles::Processor do end context 'with multiple external files are defined' do - let(:external_files) { + let(:external_files) do [ "/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml", "/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-2.yml", 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' ] - } - let(:values) { { includes: external_files, image: 'ruby:2.2'} } + end + let(:values) { { includes: external_files, image: 'ruby:2.2' } } - let(:remote_file_content) { + let(:remote_file_content) do <<-HEREDOC stages: - build - review - cleanup HEREDOC - } + end before do allow_any_instance_of(Kernel).to receive_message_chain(:open, :read).and_return(remote_file_content) @@ -124,7 +124,7 @@ describe Gitlab::Ci::ExternalFiles::Processor do end context 'when external files are defined but not valid' do - let(:values) { { includes: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2'} } + let(:values) { { includes: '/vendor/gitlab-ci-yml/template.yml', image: 'ruby:2.2' } } let(:external_file_content) { 'invalid content file ////' }