Merge remote-tracking branch 'origin/master' into artifacts-expire-date

CHANGELOG

@@ -77,6 +77,7 @@ v 8.9.0 (unreleased)
   - All classes in the Banzai::ReferenceParser namespace are now instrumented
   - Remove deprecated issues_tracker and issues_tracker_id from project model
   - Allow users to create confidential issues in private projects
+  - Measure CPU time for instrumented methods
 
 v 8.8.5 (unreleased)
   - Ensure branch cleanup regardless of whether the GitHub import process succeeds

doc/ci/docker/using_docker_images.md

@@ -23,7 +23,7 @@ To use GitLab Runner with docker you need to register a new runner to use the
 `docker` executor:
 
 ```bash
-gitlab-runner register \
+gitlab-ci-multi-runner register \
   --url "https://gitlab.com/" \
   --registration-token "PROJECT_REGISTRATION_TOKEN" \
   --description "docker-ruby-2.1" \

doc/ci/examples/php.md

@@ -263,10 +263,10 @@ terminal execute:
 
 ```bash
 # Check using docker executor
-gitlab-runner exec docker test:app
+gitlab-ci-multi-runner exec docker test:app
 
 # Check using shell executor
-gitlab-runner exec shell test:app
+gitlab-ci-multi-runner exec shell test:app
 ```
 
 ## Example project

doc/ci/runners/README.md

@@ -63,10 +63,10 @@ instance.
 Now simply register the runner as any runner:
 
 ```
-sudo gitlab-runner register
+sudo gitlab-ci-multi-runner register
 ```
 
-Shared runners are enabled by default as of GitLab 8.2, but can be disabled with the 
+Shared runners are enabled by default as of GitLab 8.2, but can be disabled with the
 `DISABLE SHARED RUNNERS` button. Previous versions of GitLab defaulted shared runners to
 disabled.
 
@@ -93,7 +93,7 @@ setup a specific runner for this project.
 To register the runner, run the command below and follow instructions:
 
 ```
-sudo gitlab-runner register
+sudo gitlab-ci-multi-runner register
 ```
 
 ###  Making an existing Shared Runner Specific

doc/container_registry/README.md

@@ -79,27 +79,8 @@ delete them.
 This feature requires GitLab 8.8 and GitLab Runner 1.2.
 
 Make sure that your GitLab Runner is configured to allow building docker images.
-You have to check the [Using Docker Build documentation](../../ci/docker/using_docker_build.md).
-
-You can use [docker:dind](https://hub.docker.com/_/docker/) to build your images,
-and this is how `.gitlab-ci.yml` should look like:
-
-```
- build_image:
-   image: docker:git
-   services:
-   - docker:dind
-   stage: build
-   script:
-     - docker login -u gitlab-ci-token -p $CI_BUILD_TOKEN registry.example.com
-     - docker build -t registry.example.com/group/project:latest .
-     - docker push registry.example.com/group/project:latest
-```
-
-You have to use the credentials of the special `gitlab-ci-token` user with its
-password stored in `$CI_BUILD_TOKEN` in order to push to the Registry connected
-to your project. This allows you to automated building and deployment of your
-Docker images.
+You have to check the [Using Docker Build documentation](../ci/docker/using_docker_build.md).
+Then see the CI documentation on [Using the GitLab Container Registry](../ci/docker/using_docker_build.md#using-the-gitlab-container-registry).
 
 ## Limitations
 

doc/development/instrumentation.md

@@ -97,15 +97,16 @@ def #{name}(#{args_signature})
   trans = Gitlab::Metrics::Instrumentation.transaction
 
   if trans
-    start    = Time.now
-    retval   = super
-    duration = (Time.now - start) * 1000.0
+    start     = Time.now
+    cpu_start = Gitlab::Metrics::System.cpu_time
+    retval    = super
+    duration  = (Time.now - start) * 1000.0
 
     if duration >= Gitlab::Metrics.method_call_threshold
-      trans.increment(:method_duration, duration)
+      cpu_duration = Gitlab::Metrics::System.cpu_time - cpu_start
 
       trans.add_metric(Gitlab::Metrics::Instrumentation::SERIES,
-                       { duration: duration },
+                       { duration: duration, cpu_duration: cpu_duration },
                        method: #{label.inspect})
     end
 

lib/banzai/pipeline/description_pipeline.rb

 module Banzai
   module Pipeline
     class DescriptionPipeline < FullPipeline
+      WHITELIST = Banzai::Filter::SanitizationFilter::LIMITED.deep_dup.merge(
+        elements: Banzai::Filter::SanitizationFilter::LIMITED[:elements] - %w(pre code img ol ul li)
+      )
+
       def self.transform_context(context)
         super(context).merge(
           # SanitizationFilter
-          whitelist: whitelist
+          whitelist: WHITELIST
         )
       end
-
-      private
-
-      def self.whitelist
-        # Descriptions are more heavily sanitized, allowing only a few elements.
-        # See http://git.io/vkuAN
-        whitelist = Banzai::Filter::SanitizationFilter::LIMITED
-        whitelist[:elements] -= %w(pre code img ol ul li)
-
-        whitelist
-      end
     end
   end
 end

lib/ci/gitlab_ci_yaml_processor.rb

@@ -2,6 +2,8 @@ module Ci
   class GitlabCiYamlProcessor
     class ValidationError < StandardError; end
 
+    include Gitlab::Ci::Config::Node::ValidationHelpers
+
     DEFAULT_STAGES = %w(build test deploy)
     DEFAULT_STAGE = 'test'
     ALLOWED_YAML_KEYS = [:before_script, :after_script, :image, :services, :types, :stages, :variables, :cache]
@@ -11,10 +13,12 @@ module Ci
     ALLOWED_CACHE_KEYS = [:key, :untracked, :paths]
     ALLOWED_ARTIFACTS_KEYS = [:name, :untracked, :paths, :when, :expire_in]
 
-    attr_reader :before_script, :after_script, :image, :services, :path, :cache
+    attr_reader :after_script, :image, :services, :path, :cache
 
     def initialize(config, path = nil)
-      @config = Gitlab::Ci::Config.new(config).to_hash
+      @ci_config = Gitlab::Ci::Config.new(config)
+      @config = @ci_config.to_hash
+
       @path = path
 
       initial_parsing
@@ -52,7 +56,6 @@ module Ci
     private
 
     def initial_parsing
-      @before_script = @config[:before_script] || []
       @after_script = @config[:after_script]
       @image = @config[:image]
       @services = @config[:services]
@@ -80,7 +83,7 @@ module Ci
       {
         stage_idx: stages.index(job[:stage]),
         stage: job[:stage],
-        commands: [job[:before_script] || @before_script, job[:script]].flatten.join("\n"),
+        commands: [job[:before_script] || [@ci_config.before_script], job[:script]].flatten.compact.join("\n"),
         tag_list: job[:tags] || [],
         name: name,
         only: job[:only],
@@ -99,6 +102,10 @@ module Ci
     end
 
     def validate!
+      unless @ci_config.valid?
+        raise ValidationError, @ci_config.errors.first
+      end
+
       validate_global!
 
       @jobs.each do |name, job|
@@ -109,10 +116,6 @@ module Ci
     end
 
     def validate_global!
-      unless validate_array_of_strings(@before_script)
-        raise ValidationError, "before_script should be an array of strings"
-      end
-
       unless @after_script.nil? || validate_array_of_strings(@after_script)
         raise ValidationError, "after_script should be an array of strings"
       end
@@ -304,28 +307,6 @@ module Ci
       end
     end
 
-    def validate_duration(value)
-      value.is_a?(String) && ChronicDuration.parse(value)
-    rescue ChronicDuration::DurationParseError
-      false
-    end
-
-    def validate_array_of_strings(values)
-      values.is_a?(Array) && values.all? { |value| validate_string(value) }
-    end
-
-    def validate_variables(variables)
-      variables.is_a?(Hash) && variables.all? { |key, value| validate_string(key) && validate_string(value) }
-    end
-
-    def validate_string(value)
-      value.is_a?(String) || value.is_a?(Symbol)
-    end
-
-    def validate_boolean(value)
-      value.in?([true, false])
-    end
-
     def process?(only_params, except_params, ref, tag, trigger_request)
       if only_params.present?
         return false unless matching?(only_params, ref, tag, trigger_request)

lib/gitlab/ci/config.rb

 module Gitlab
   module Ci
+    ##
+    # Base GitLab CI Configuration facade
+    #
     class Config
-      class LoaderError < StandardError; end
+      delegate :valid?, :errors, to: :@global
+
+      ##
+      # Temporary delegations that should be removed after refactoring
+      #
+      delegate :before_script, to: :@global
 
       def initialize(config)
-        loader = Loader.new(config)
-        @config = loader.load!
+        @config = Loader.new(config).load!
+
+        @global = Node::Global.new(@config)
+        @global.process!
       end
 
       def to_hash

lib/gitlab/ci/config/node/configurable.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        ##
+        # This mixin is responsible for adding DSL, which purpose is to
+        # simplifly process of adding child nodes.
+        #
+        # This can be used only if parent node is a configuration entry that
+        # holds a hash as a configuration value, for example:
+        #
+        # job:
+        #   script: ...
+        #   artifacts: ...
+        #
+        module Configurable
+          extend ActiveSupport::Concern
+
+          def allowed_nodes
+            self.class.allowed_nodes || {}
+          end
+
+          private
+
+          def prevalidate!
+            unless @value.is_a?(Hash)
+              @errors << 'should be a configuration entry with hash value'
+            end
+          end
+
+          def create_node(key, factory)
+            factory.with(value: @value[key])
+            factory.nullify! unless @value.has_key?(key)
+            factory.create!
+          end
+
+          class_methods do
+            def allowed_nodes
+              Hash[@allowed_nodes.map { |key, factory| [key, factory.dup] }]
+            end
+
+            private
+
+            def allow_node(symbol, entry_class, metadata)
+              factory = Node::Factory.new(entry_class)
+                .with(description: metadata[:description])
+
+              define_method(symbol) do
+                raise Entry::InvalidError unless valid?
+
+                @nodes[symbol].try(:value)
+              end
+
+              (@allowed_nodes ||= {}).merge!(symbol => factory)
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/node/entry.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        ##
+        # Base abstract class for each configuration entry node.
+        #
+        class Entry
+          class InvalidError < StandardError; end
+
+          attr_accessor :description
+
+          def initialize(value)
+            @value = value
+            @nodes = {}
+            @errors = []
+
+            prevalidate!
+          end
+
+          def process!
+            return if leaf?
+            return unless valid?
+
+            compose!
+
+            nodes.each(&:process!)
+            nodes.each(&:validate!)
+          end
+
+          def nodes
+            @nodes.values
+          end
+
+          def valid?
+            errors.none?
+          end
+
+          def leaf?
+            allowed_nodes.none?
+          end
+
+          def errors
+            @errors + nodes.map(&:errors).flatten
+          end
+
+          def allowed_nodes
+            {}
+          end
+
+          def validate!
+            raise NotImplementedError
+          end
+
+          def value
+            raise NotImplementedError
+          end
+
+          private
+
+          def prevalidate!
+          end
+
+          def compose!
+            allowed_nodes.each do |key, essence|
+              @nodes[key] = create_node(key, essence)
+            end
+          end
+
+          def create_node(key, essence)
+            raise NotImplementedError
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/node/factory.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        ##
+        # Factory class responsible for fabricating node entry objects.
+        #
+        # It uses Fluent Interface pattern to set all necessary attributes.
+        #
+        class Factory
+          class InvalidFactory < StandardError; end
+
+          def initialize(entry_class)
+            @entry_class = entry_class
+            @attributes = {}
+          end
+
+          def with(attributes)
+            @attributes.merge!(attributes)
+            self
+          end
+
+          def nullify!
+            @entry_class = Node::Null
+            self
+          end
+
+          def create!
+            raise InvalidFactory unless @attributes.has_key?(:value)
+
+            @entry_class.new(@attributes[:value]).tap do |entry|
+              entry.description = @attributes[:description]
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/node/global.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        ##
+        # This class represents a global entry - root node for entire
+        # GitLab CI Configuration file.
+        #
+        class Global < Entry
+          include Configurable
+
+          allow_node :before_script, Script,
+            description: 'Script that will be executed before each job.'
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/node/null.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        ##
+        # This class represents a configuration entry that is not being used
+        # in configuration file.
+        #
+        # This implements Null Object pattern.
+        #
+        class Null < Entry
+          def value
+            nil
+          end
+
+          def validate!
+            nil
+          end
+
+          def method_missing(*)
+            nil
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/node/script.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        ##
+        # Entry that represents a script.
+        #
+        # Each element in the value array is a command that will be executed
+        # by GitLab Runner. Currently we concatenate these commands with
+        # new line character as a separator, what is compatible with
+        # implementation in Runner.
+        #
+        class Script < Entry
+          include ValidationHelpers
+
+          def value
+            @value.join("\n")
+          end
+
+          def validate!
+            unless validate_array_of_strings(@value)
+              @errors << 'before_script should be an array of strings'
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/node/validation_helpers.rb

+module Gitlab
+  module Ci
+    class Config
+      module Node
+        module ValidationHelpers
+          private
+
+          def validate_duration(value)
+            value.is_a?(String) && ChronicDuration.parse(value)
+          rescue ChronicDuration::DurationParseError
+            false
+          end
+
+          def validate_array_of_strings(values)
+            values.is_a?(Array) && values.all? { |value| validate_string(value) }
+          end
+
+          def validate_variables(variables)
+            variables.is_a?(Hash) &&
+              variables.all? { |key, value| validate_string(key) && validate_string(value) }
+          end
+
+          def validate_string(value)
+            value.is_a?(String) || value.is_a?(Symbol)
+          end
+
+          def validate_boolean(value)
+            value.in?([true, false])
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/metrics/instrumentation.rb

@@ -149,13 +149,16 @@ module Gitlab
             trans = Gitlab::Metrics::Instrumentation.transaction
 
             if trans
-              start    = Time.now
-              retval   = super
-              duration = (Time.now - start) * 1000.0
+              start     = Time.now
+              cpu_start = Gitlab::Metrics::System.cpu_time
+              retval    = super
+              duration  = (Time.now - start) * 1000.0
 
               if duration >= Gitlab::Metrics.method_call_threshold
+                cpu_duration = Gitlab::Metrics::System.cpu_time - cpu_start
+
                 trans.add_metric(Gitlab::Metrics::Instrumentation::SERIES,
-                                 { duration: duration },
+                                 { duration: duration, cpu_duration: cpu_duration },
                                  method: #{label.inspect})
               end
 

spec/lib/gitlab/ci/config/node/configurable_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Configurable do
+  let(:node) { Class.new }
+
+  before do
+    node.include(described_class)
+  end
+
+  describe 'allowed nodes' do
+    before do
+      node.class_eval do
+        allow_node :object, Object, description: 'test object'
+      end
+    end
+
+    describe '#allowed_nodes' do
+      it 'has valid allowed nodes' do
+        expect(node.allowed_nodes).to include :object
+      end
+
+      it 'creates a node factory' do
+        expect(node.allowed_nodes[:object])
+          .to be_an_instance_of Gitlab::Ci::Config::Node::Factory
+      end
+
+      it 'returns a duplicated factory object' do
+        first_factory = node.allowed_nodes[:object]
+        second_factory = node.allowed_nodes[:object]
+
+        expect(first_factory).not_to be_equal(second_factory)
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config/node/factory_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Factory do
+  describe '#create!' do
+    let(:factory) { described_class.new(entry_class) }
+    let(:entry_class) { Gitlab::Ci::Config::Node::Script }
+
+    context 'when value setting value' do
+      it 'creates entry with valid value' do
+        entry = factory
+          .with(value: ['ls', 'pwd'])
+          .create!
+
+        expect(entry.value).to eq "ls\npwd"
+      end
+
+      context 'when setting description' do
+        it 'creates entry with description' do
+          entry = factory
+            .with(value: ['ls', 'pwd'])
+            .with(description: 'test description')
+            .create!
+
+          expect(entry.value).to eq "ls\npwd"
+          expect(entry.description).to eq 'test description'
+        end
+      end
+    end
+
+    context 'when not setting value' do
+      it 'raises error' do
+        expect { factory.create! }.to raise_error(
+          Gitlab::Ci::Config::Node::Factory::InvalidFactory
+        )
+      end
+    end
+
+    context 'when creating a null entry' do
+      it 'creates a null entry' do
+        entry = factory
+          .with(value: nil)
+          .nullify!
+          .create!
+
+        expect(entry).to be_an_instance_of Gitlab::Ci::Config::Node::Null
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config/node/global_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Global do
+  let(:global) { described_class.new(hash) }
+
+  describe '#allowed_nodes' do
+    it 'can contain global config keys' do
+      expect(global.allowed_nodes).to include :before_script
+    end
+
+    it 'returns a hash' do
+      expect(global.allowed_nodes).to be_a Hash
+    end
+  end
+
+  context 'when hash is valid' do
+    let(:hash) do
+      { before_script: ['ls', 'pwd'] }
+    end
+
+    describe '#process!' do
+      before { global.process! }
+
+      it 'creates nodes hash' do
+        expect(global.nodes).to be_an Array
+      end
+
+      it 'creates node object for each entry' do
+        expect(global.nodes.count).to eq 1
+      end
+
+      it 'creates node object using valid class' do
+        expect(global.nodes.first)
+          .to be_an_instance_of Gitlab::Ci::Config::Node::Script
+      end
+
+      it 'sets correct description for nodes' do
+        expect(global.nodes.first.description)
+          .to eq 'Script that will be executed before each job.'
+      end
+    end
+
+    describe '#leaf?' do
+      it 'is not leaf' do
+        expect(global).not_to be_leaf
+      end
+    end
+
+    describe '#before_script' do
+      context 'when processed' do
+        before { global.process! }
+
+        it 'returns correct script' do
+          expect(global.before_script).to eq "ls\npwd"
+        end
+      end
+
+      context 'when not processed' do
+        it 'returns nil' do
+          expect(global.before_script).to be nil
+        end
+      end
+    end
+  end
+
+  context 'when hash is not valid' do
+    before { global.process! }
+
+    let(:hash) do
+      { before_script: 'ls' }
+    end
+
+    describe '#valid?' do
+      it 'is not valid' do
+        expect(global).not_to be_valid
+      end
+    end
+
+    describe '#errors' do
+      it 'reports errors from child nodes' do
+        expect(global.errors)
+          .to include 'before_script should be an array of strings'
+      end
+    end
+
+    describe '#before_script' do
+      it 'raises error' do
+        expect { global.before_script }.to raise_error(
+          Gitlab::Ci::Config::Node::Entry::InvalidError
+        )
+      end
+    end
+  end
+
+  context 'when value is not a hash' do
+    let(:hash) { [] }
+
+    describe '#valid?' do
+      it 'is not valid' do
+        expect(global).not_to be_valid
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config/node/null_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Null do
+  let(:entry) { described_class.new(nil) }
+
+  describe '#leaf?' do
+    it 'is leaf node' do
+      expect(entry).to be_leaf
+    end
+  end
+
+  describe '#any_method' do
+    it 'responds with nil' do
+      expect(entry.any_method).to be nil
+    end
+  end
+
+  describe '#value' do
+    it 'returns nil' do
+      expect(entry.value).to be nil
+    end
+  end
+end

spec/lib/gitlab/ci/config/node/script_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Node::Script do
+  let(:entry) { described_class.new(value) }
+
+  describe '#validate!' do
+    before { entry.validate! }
+
+    context 'when entry value is correct' do
+      let(:value) { ['ls', 'pwd'] }
+
+      describe '#value' do
+        it 'returns concatenated command' do
+          expect(entry.value).to eq "ls\npwd"
+        end
+      end
+
+      describe '#errors' do
+        it 'does not append errors' do
+          expect(entry.errors).to be_empty
+        end
+      end
+
+      describe '#valid?' do
+        it 'is valid' do
+          expect(entry).to be_valid
+        end
+      end
+    end
+
+    context 'when entry value is not correct' do
+      let(:value) { 'ls' }
+
+      describe '#errors' do
+        it 'saves errors' do
+          expect(entry.errors)
+            .to include /should be an array of strings/
+        end
+      end
+
+      describe '#valid?' do
+        it 'is not valid' do
+          expect(entry).not_to be_valid
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config_spec.rb

@@ -29,17 +29,43 @@ describe Gitlab::Ci::Config do
 
         expect(config.to_hash).to eq hash
       end
+
+      describe '#valid?' do
+        it 'is valid' do
+          expect(config).to be_valid
+        end
+
+        it 'has no errors' do
+          expect(config.errors).to be_empty
+        end
+      end
     end
 
     context 'when config is invalid' do
-      let(:yml) { '// invalid' }
-
-      describe '.new' do
-        it 'raises error' do
-          expect { config }.to raise_error(
-            Gitlab::Ci::Config::Loader::FormatError,
-            /Invalid configuration format/
-          )
+      context 'when yml is incorrect' do
+        let(:yml) { '// invalid' }
+
+        describe '.new' do
+          it 'raises error' do
+            expect { config }.to raise_error(
+              Gitlab::Ci::Config::Loader::FormatError,
+              /Invalid configuration format/
+            )
+          end
+        end
+      end
+
+      context 'when config logic is incorrect' do
+        let(:yml) { 'before_script: "ls"' }
+
+        describe '#valid?' do
+          it 'is not valid' do
+            expect(config).not_to be_valid
+          end
+
+          it 'has errors' do
+            expect(config.errors).not_to be_empty
+          end
         end
       end
     end

spec/lib/gitlab/metrics/instrumentation_spec.rb

@@ -57,7 +57,7 @@ describe Gitlab::Metrics::Instrumentation do
           and_return(transaction)
 
         expect(transaction).to receive(:add_metric).
-          with(described_class::SERIES, an_instance_of(Hash),
+          with(described_class::SERIES, hash_including(:duration, :cpu_duration),
                method: 'Dummy.foo')
 
         @dummy.foo
@@ -137,7 +137,7 @@ describe Gitlab::Metrics::Instrumentation do
           and_return(transaction)
 
         expect(transaction).to receive(:add_metric).
-          with(described_class::SERIES, an_instance_of(Hash),
+          with(described_class::SERIES, hash_including(:duration, :cpu_duration),
                method: 'Dummy#bar')
 
         @dummy.new.bar