Merge branch 'kamil-refactor-ci-builds-v5' into 'master'

Use BuildMetadata to store build configuration in JSONB form

See merge request gitlab-org/gitlab-ce!21499

app/models/ci/build.rb

@@ -8,10 +8,15 @@ module Ci
     include ObjectStorage::BackgroundMove
     include Presentable
     include Importable
+    include IgnorableColumn
     include Gitlab::Utils::StrongMemoize
     include Deployable
     include HasRef
 
+    BuildArchivedError = Class.new(StandardError)
+
+    ignore_column :commands
+
     belongs_to :project, inverse_of: :builds
     belongs_to :runner
     belongs_to :trigger_request
@@ -31,7 +36,7 @@ module Ci
       has_one :"job_artifacts_#{key}", -> { where(file_type: value) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
     end
 
-    has_one :metadata, class_name: 'Ci::BuildMetadata'
+    has_one :metadata, class_name: 'Ci::BuildMetadata', autosave: true
     has_one :runner_session, class_name: 'Ci::BuildRunnerSession', validate: true, inverse_of: :build
 
     accepts_nested_attributes_for :runner_session
@@ -273,11 +278,14 @@ module Ci
 
     # degenerated build is one that cannot be run by Runner
     def degenerated?
-      self.options.nil?
+      self.options.blank?
     end
 
     def degenerate!
-      self.update!(options: nil, yaml_variables: nil, commands: nil)
+      Build.transaction do
+        self.update!(options: nil, yaml_variables: nil)
+        self.metadata&.destroy
+      end
     end
 
     def archived?
@@ -624,11 +632,23 @@ module Ci
     end
 
     def when
-      read_attribute(:when) || build_attributes_from_config[:when] || 'on_success'
+      read_attribute(:when) || 'on_success'
+    end
+
+    def options
+      read_metadata_attribute(:options, :config_options, {})
     end
 
     def yaml_variables
-      read_attribute(:yaml_variables) || build_attributes_from_config[:yaml_variables] || []
+      read_metadata_attribute(:yaml_variables, :config_variables, [])
+    end
+
+    def options=(value)
+      write_metadata_attribute(:options, :config_options, value)
+    end
+
+    def yaml_variables=(value)
+      write_metadata_attribute(:yaml_variables, :config_variables, value)
     end
 
     def user_variables
@@ -904,8 +924,11 @@ module Ci
     # have the old integer only format. This method returns the retry option
     # normalized as a hash in 11.5+ format.
     def normalized_retry
-      value = options&.dig(:retry)
-      value.is_a?(Integer) ? { max: value } : value.to_h
+      strong_memoize(:normalized_retry) do
+        value = options&.dig(:retry)
+        value = value.is_a?(Integer) ? { max: value } : value.to_h
+        value.with_indifferent_access
+      end
     end
 
     def build_attributes_from_config
@@ -929,5 +952,20 @@ module Ci
     def project_destroyed?
       project.pending_delete?
     end
+
+    def read_metadata_attribute(legacy_key, metadata_key, default_value = nil)
+      read_attribute(legacy_key) || metadata&.read_attribute(metadata_key) || default_value
+    end
+
+    def write_metadata_attribute(legacy_key, metadata_key, value)
+      # save to metadata or this model depending on the state of feature flag
+      if Feature.enabled?(:ci_build_metadata_config)
+        ensure_metadata.write_attribute(metadata_key, value)
+        write_attribute(legacy_key, nil)
+      else
+        write_attribute(legacy_key, value)
+        metadata&.write_attribute(metadata_key, nil)
+      end
+    end
   end
 end

app/models/ci/build_metadata.rb

@@ -13,8 +13,12 @@ module Ci
     belongs_to :build, class_name: 'Ci::Build'
     belongs_to :project
 
+    before_create :set_build_project
+
     validates :build, presence: true
-    validates :project, presence: true
+
+    serialize :config_options, Serializers::JSON # rubocop:disable Cop/ActiveRecordSerialize
+    serialize :config_variables, Serializers::JSON # rubocop:disable Cop/ActiveRecordSerialize
 
     chronic_duration_attr_reader :timeout_human_readable, :timeout
 
@@ -33,5 +37,11 @@ module Ci
 
       update(timeout: timeout, timeout_source: timeout_source)
     end
+
+    private
+
+    def set_build_project
+      self.project_id ||= self.build.project_id
+    end
   end
 end

app/services/ci/retry_build_service.rb

@@ -2,7 +2,7 @@
 
 module Ci
   class RetryBuildService < ::BaseService
-    CLONE_ACCESSORS = %i[pipeline project ref tag options commands name
+    CLONE_ACCESSORS = %i[pipeline project ref tag options name
                          allow_failure stage stage_id stage_idx trigger_request
                          yaml_variables when environment coverage_regex
                          description tag_list protected].freeze

app/views/projects/ci/lints/_create.html.haml

@@ -13,20 +13,23 @@
       %tbody
         - @stages.each do |stage|
           - @builds.select { |build| build[:stage] == stage }.each do |build|
+            - job = @jobs[build[:name].to_sym]
             %tr
               %td #{stage.capitalize} Job - #{build[:name]}
               %td
-                %pre= build[:commands]
+                %pre= job[:before_script].to_a.join('\n')
+                %pre= job[:script].to_a.join('\n')
+                %pre= job[:after_script].to_a.join('\n')
 
                 %br
                 %b Tag list:
                 = build[:tag_list].to_a.join(", ")
                 %br
                 %b Only policy:
-                = @jobs[build[:name].to_sym][:only].to_a.join(", ")
+                = job[:only].to_a.join(", ")
                 %br
                 %b Except policy:
-                = @jobs[build[:name].to_sym][:except].to_a.join(", ")
+                = job[:except].to_a.join(", ")
                 %br
                 %b Environment:
                 = build[:environment]

config/initializers/ar_mysql_jsonb_support.rb

+# frozen_string_literal: true
+
+require 'active_record/connection_adapters/abstract_mysql_adapter'
+require 'active_record/connection_adapters/mysql/schema_definitions'
+
+# MySQL (5.6) and MariaDB (10.1) are currently supported versions within GitLab,
+# Since they do not support native `json` datatype we force to emulate it as `text`
+
+if Gitlab::Database.mysql?
+  module ActiveRecord
+    module ConnectionAdapters
+      class AbstractMysqlAdapter
+        JSON_DATASIZE = 1.megabyte
+
+        NATIVE_DATABASE_TYPES.merge!(
+          json: { name: "text", limit: JSON_DATASIZE },
+          jsonb: { name: "text", limit: JSON_DATASIZE }
+        )
+      end
+
+      module MySQL
+        module ColumnMethods
+          # We add `jsonb` helper, as `json` is already defined for `MySQL` since Rails 5
+          def jsonb(*args, **options)
+            args.each { |name| column(name, :json, options) }
+          end
+        end
+      end
+    end
+  end
+end

db/fixtures/development/14_pipelines.rb

@@ -102,14 +102,15 @@ class Gitlab::Seeder::Pipelines
     []
   end
 
-
   def create_pipeline!(project, ref, commit)
     project.ci_pipelines.create!(sha: commit.id, ref: ref, source: :push)
   end
 
   def build_create!(pipeline, opts = {})
     attributes = job_attributes(pipeline, opts)
-      .merge(commands: '$ build command')
+
+    attributes[:options] ||= {}
+    attributes[:options][:script] = 'build command'
 
     Ci::Build.create!(attributes).tap do |build|
       # We need to set build trace and artifacts after saving a build

db/migrate/20181219145521_add_options_to_build_metadata.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddOptionsToBuildMetadata < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :ci_builds_metadata, :config_options, :jsonb
+    add_column :ci_builds_metadata, :config_variables, :jsonb
+  end
+end

db/schema.rb

@@ -374,6 +374,8 @@ ActiveRecord::Schema.define(version: 20190103140724) do
     t.integer "project_id", null: false
     t.integer "timeout"
     t.integer "timeout_source", default: 1, null: false
+    t.jsonb "config_options"
+    t.jsonb "config_variables"
     t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true, using: :btree
     t.index ["project_id"], name: "index_ci_builds_metadata_on_project_id", using: :btree
   end

doc/development/migration_style_guide.md

@@ -325,6 +325,31 @@ This ensures all timestamps have a time zone specified. This in turn means exist
 suddenly use a different timezone when the system's timezone changes. It also makes it very clear which
 timezone was used in the first place.
 
+## Storing JSON in database
+
+The Rails 5 natively supports `JSONB` (binary JSON) column type.
+Example migration adding this column:
+
+```ruby
+class AddOptionsToBuildMetadata < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def change
+    add_column :ci_builds_metadata, :config_options, :jsonb
+  end
+end
+```
+
+On MySQL the `JSON` and `JSONB` is translated to `TEXT 1MB`, as `JSONB` is PostgreSQL only feature.
+
+For above reason you have to use a serializer to provide a translation layer
+in order to support PostgreSQL and MySQL seamlessly:
+
+```ruby
+class BuildMetadata
+  serialize :config_options, Serializers::JSON # rubocop:disable Cop/ActiveRecordSerialize
+end
+```
 
 ## Testing
 

lib/gitlab/ci/build/step.rb

@@ -15,7 +15,6 @@ module Gitlab
           def from_commands(job)
             self.new(:script).tap do |step|
               step.script = job.options[:before_script].to_a + job.options[:script].to_a
-              step.script = job.commands.split("\n") if step.script.empty?
               step.timeout = job.metadata_timeout
               step.when = WHEN_ON_SUCCESS
             end

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

@@ -95,7 +95,7 @@ module Gitlab
 
           helpers :before_script, :script, :stage, :type, :after_script,
                   :cache, :image, :services, :only, :except, :variables,
-                  :artifacts, :commands, :environment, :coverage, :retry,
+                  :artifacts, :environment, :coverage, :retry,
                   :parallel
 
           attributes :script, :tags, :allow_failure, :when, :dependencies,
@@ -121,10 +121,6 @@ module Gitlab
             @config.merge(to_hash.compact)
           end
 
-          def commands
-            (before_script_value.to_a + script_value.to_a).join("\n")
-          end
-
           def manual_action?
             self.when == 'manual'
           end
@@ -156,7 +152,6 @@ module Gitlab
             { name: name,
               before_script: before_script_value,
               script: script_value,
-              commands: commands,
               image: image_value,
               services: services_value,
               stage: stage_value,

lib/gitlab/ci/yaml_processor.rb

@@ -33,7 +33,6 @@ module Gitlab
 
         { stage_idx: @stages.index(job[:stage]),
           stage: job[:stage],
-          commands: job[:commands],
           tag_list: job[:tags] || [],
           name: job[:name].to_s,
           allow_failure: job[:ignore],

lib/gitlab/import_export/import_export.yml

@@ -148,6 +148,7 @@ excluded_attributes:
     - :when
     - :artifacts_file
     - :artifacts_metadata
+    - :commands
   push_event_payload:
     - :event_id
   project_badges:

lib/gitlab/import_export/relation_factory.rb

@@ -150,6 +150,7 @@ module Gitlab
         if BUILD_MODELS.include?(@relation_name)
           @relation_hash.delete('trace') # old export files have trace
           @relation_hash.delete('token')
+          @relation_hash.delete('commands')
 
           imported_object
         elsif @relation_name == :merge_requests

lib/gitlab/utils.rb

@@ -115,5 +115,15 @@ module Gitlab
 
       string_or_array.split(',').map(&:strip)
     end
+
+    def deep_indifferent_access(data)
+      if data.is_a?(Array)
+        data.map(&method(:deep_indifferent_access))
+      elsif data.is_a?(Hash)
+        data.with_indifferent_access
+      else
+        data
+      end
+    end
   end
 end

lib/serializers/json.rb

+# frozen_string_literal: true
+
+module Serializers
+  # This serializer exports data as JSON,
+  # it is designed to be used with interwork compatibility between MySQL and PostgreSQL
+  # implementations, as used version of MySQL does not support native json type
+  #
+  # Secondly, the loader makes the resulting hash to have deep indifferent access
+  class JSON
+    class << self
+      def dump(obj)
+        # MySQL stores data as text
+        # look at ./config/initializers/ar_mysql_jsonb_support.rb
+        if Gitlab::Database.mysql?
+          obj = ActiveSupport::JSON.encode(obj)
+        end
+
+        obj
+      end
+
+      def load(data)
+        return if data.nil?
+
+        # On MySQL we store data as text
+        # look at ./config/initializers/ar_mysql_jsonb_support.rb
+        if Gitlab::Database.mysql?
+          data = ActiveSupport::JSON.decode(data)
+        end
+
+        Gitlab::Utils.deep_indifferent_access(data)
+      end
+    end
+  end
+end

spec/factories/ci/builds.rb

@@ -7,7 +7,6 @@ FactoryBot.define do
     stage_idx 0
     ref 'master'
     tag false
-    commands 'ls -a'
     protected false
     created_at 'Di 29. Okt 09:50:00 CET 2013'
     pending
@@ -15,7 +14,8 @@ FactoryBot.define do
     options do
       {
         image: 'ruby:2.1',
-        services: ['postgres']
+        services: ['postgres'],
+        script: ['ls -a']
       }
     end
 
@@ -28,7 +28,6 @@ FactoryBot.define do
     pipeline factory: :ci_pipeline
 
     trait :degenerated do
-      commands nil
       options nil
       yaml_variables nil
     end
@@ -95,33 +94,53 @@ FactoryBot.define do
 
     trait :teardown_environment do
       environment 'staging'
-      options environment: { name: 'staging',
-                             action: 'stop',
-                             url: 'http://staging.example.com/$CI_JOB_NAME' }
+      options do
+        {
+          script: %w(ls),
+          environment: { name: 'staging',
+                         action: 'stop',
+                         url: 'http://staging.example.com/$CI_JOB_NAME' }
+        }
+      end
     end
 
     trait :deploy_to_production do
       environment 'production'
 
-      options environment: { name: 'production',
-                             url: 'http://prd.example.com/$CI_JOB_NAME' }
+      options do
+        {
+          script: %w(ls),
+          environment: { name: 'production',
+                         url: 'http://prd.example.com/$CI_JOB_NAME' }
+        }
+      end
     end
 
     trait :start_review_app do
       environment 'review/$CI_COMMIT_REF_NAME'
 
-      options environment: { name: 'review/$CI_COMMIT_REF_NAME',
-                             url: 'http://staging.example.com/$CI_JOB_NAME',
-                             on_stop: 'stop_review_app' }
+      options do
+        {
+          script: %w(ls),
+          environment: { name: 'review/$CI_COMMIT_REF_NAME',
+                         url: 'http://staging.example.com/$CI_JOB_NAME',
+                         on_stop: 'stop_review_app' }
+        }
+      end
     end
 
     trait :stop_review_app do
       name 'stop_review_app'
       environment 'review/$CI_COMMIT_REF_NAME'
 
-      options environment: { name: 'review/$CI_COMMIT_REF_NAME',
-                             url: 'http://staging.example.com/$CI_JOB_NAME',
-                             action: 'stop' }
+      options do
+        {
+          script: %w(ls),
+          environment: { name: 'review/$CI_COMMIT_REF_NAME',
+                         url: 'http://staging.example.com/$CI_JOB_NAME',
+                         action: 'stop' }
+        }
+      end
     end
 
     trait :allowed_to_fail do
@@ -142,7 +161,13 @@ FactoryBot.define do
 
     trait :schedulable do
       self.when 'delayed'
-      options start_in: '1 minute'
+
+      options do
+        {
+          script: ['ls -a'],
+          start_in: '1 minute'
+        }
+      end
     end
 
     trait :actionable do
@@ -265,6 +290,7 @@ FactoryBot.define do
         {
             image: { name: 'ruby:2.1', entrypoint: '/bin/sh' },
             services: ['postgres', { name: 'docker:stable-dind', entrypoint: '/bin/sh', command: 'sleep 30', alias: 'docker' }],
+            script: %w(echo),
             after_script: %w(ls date),
             artifacts: {
                 name: 'artifacts_file',

spec/features/merge_request/user_sees_mini_pipeline_graph_spec.rb

@@ -5,7 +5,7 @@ describe 'Merge request < User sees mini pipeline graph', :js do
   let(:user) { project.creator }
   let(:merge_request) { create(:merge_request, source_project: project, head_pipeline: pipeline) }
   let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: 'master', status: 'running', sha: project.commit.id) }
-  let(:build) { create(:ci_build, pipeline: pipeline, stage: 'test', commands: 'test') }
+  let(:build) { create(:ci_build, pipeline: pipeline, stage: 'test') }
 
   before do
     build.run

spec/features/projects/environments/environments_spec.rb

@@ -272,8 +272,7 @@ describe 'Environments page', :js do
           create(:ci_build, :scheduled,
                  pipeline: pipeline,
                  name: 'delayed job',
-                 stage: 'test',
-                 commands: 'test')
+                 stage: 'test')
         end
 
         let!(:deployment) do
@@ -304,8 +303,7 @@ describe 'Environments page', :js do
             create(:ci_build, :expired_scheduled,
                    pipeline: pipeline,
                    name: 'delayed job',
-                   stage: 'test',
-                   commands: 'test')
+                   stage: 'test')
           end
 
           it "shows 00:00:00 as the remaining time" do

spec/features/projects/pipelines/pipeline_spec.rb

@@ -18,7 +18,7 @@ describe 'Pipeline', :js do
 
     let!(:build_failed) do
       create(:ci_build, :failed,
-             pipeline: pipeline, stage: 'test', name: 'test', commands: 'test')
+             pipeline: pipeline, stage: 'test', name: 'test')
     end
 
     let!(:build_running) do

spec/features/projects/pipelines/pipelines_spec.rb

@@ -109,8 +109,7 @@ describe 'Pipelines', :js do
       context 'when pipeline is cancelable' do
         let!(:build) do
           create(:ci_build, pipeline: pipeline,
-                            stage: 'test',
-                            commands: 'test')
+                            stage: 'test')
         end
 
         before do
@@ -140,8 +139,7 @@ describe 'Pipelines', :js do
       context 'when pipeline is retryable' do
         let!(:build) do
           create(:ci_build, pipeline: pipeline,
-                            stage: 'test',
-                            commands: 'test')
+                            stage: 'test')
         end
 
         before do
@@ -202,8 +200,7 @@ describe 'Pipelines', :js do
           create(:ci_build, :manual,
             pipeline: pipeline,
             name: 'manual build',
-            stage: 'test',
-            commands: 'test')
+            stage: 'test')
         end
 
         before do
@@ -237,8 +234,7 @@ describe 'Pipelines', :js do
           create(:ci_build, :scheduled,
             pipeline: pipeline,
             name: 'delayed job',
-            stage: 'test',
-            commands: 'test')
+            stage: 'test')
         end
 
         before do
@@ -262,8 +258,7 @@ describe 'Pipelines', :js do
             create(:ci_build, :expired_scheduled,
               pipeline: pipeline,
               name: 'delayed job',
-              stage: 'test',
-              commands: 'test')
+              stage: 'test')
           end
 
           it "shows 00:00:00 as the remaining time" do

spec/javascripts/fixtures/jobs.rb

@@ -14,8 +14,7 @@ describe Projects::JobsController, '(JavaScript fixtures)', type: :controller do
     create(:ci_build, :scheduled,
            pipeline: pipeline,
            name: 'delayed job',
-           stage: 'test',
-           commands: 'test')
+           stage: 'test')
   end
 
   render_views

spec/lib/gitlab/background_migration/populate_external_pipeline_source_spec.rb

@@ -5,6 +5,11 @@ require 'spec_helper'
 describe Gitlab::BackgroundMigration::PopulateExternalPipelineSource, :migration, schema: 20180916011959 do
   let(:migration) { described_class.new }
 
+  before do
+    # This migration was created before we introduced metadata configs
+    stub_feature_flags(ci_build_metadata_config: false)
+  end
+
   let!(:internal_pipeline) { create(:ci_pipeline, source: :web) }
   let(:pipelines) { [internal_pipeline, unknown_pipeline].map(&:id) }
 

spec/lib/gitlab/ci/build/step_spec.rb

@@ -18,13 +18,6 @@ describe Gitlab::Ci::Build::Step do
       end
     end
 
-    context 'when commands are specified' do
-      it_behaves_like 'has correct script' do
-        let(:job) { create(:ci_build, :no_options, commands: "ls -la\ndate") }
-        let(:script) { ['ls -la', 'date'] }
-      end
-    end
-
     context 'when script option is specified' do
       it_behaves_like 'has correct script' do
         let(:job) { create(:ci_build, :no_options, options: { script: ["ls -la\necho aaa", "date"] }) }
@@ -62,7 +55,7 @@ describe Gitlab::Ci::Build::Step do
     end
 
     context 'when after_script is not empty' do
-      let(:job) { create(:ci_build, options: { after_script: ['ls -la', 'date'] }) }
+      let(:job) { create(:ci_build, options: { script: ['bash'], after_script: ['ls -la', 'date'] }) }
 
       it 'fabricates an object' do
         expect(subject.name).to eq(:after_script)

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

@@ -153,7 +153,6 @@ describe Gitlab::Ci::Config::Entry::Global do
               rspec: { name: :rspec,
                        script: %w[rspec ls],
                        before_script: %w(ls pwd),
-                       commands: "ls\npwd\nrspec\nls",
                        image: { name: 'ruby:2.2' },
                        services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                        stage: 'test',
@@ -166,7 +165,6 @@ describe Gitlab::Ci::Config::Entry::Global do
               spinach: { name: :spinach,
                          before_script: [],
                          script: %w[spinach],
-                         commands: 'spinach',
                          image: { name: 'ruby:2.2' },
                          services: [{ name: 'postgres:9.1' }, { name: 'mysql:5.5' }],
                          stage: 'test',

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -255,7 +255,6 @@ describe Gitlab::Ci::Config::Entry::Job do
             .to eq(name: :rspec,
                    before_script: %w[ls pwd],
                    script: %w[rspec],
-                   commands: "ls\npwd\nrspec",
                    stage: 'test',
                    ignore: false,
                    after_script: %w[cleanup],
@@ -264,16 +263,6 @@ describe Gitlab::Ci::Config::Entry::Job do
         end
       end
     end
-
-    describe '#commands' do
-      let(:config) do
-        { before_script: %w[ls pwd], script: 'rspec' }
-      end
-
-      it 'returns a string of commands concatenated with new line character' do
-        expect(entry.commands).to eq "ls\npwd\nrspec"
-      end
-    end
   end
 
   describe '#manual_action?' do

spec/lib/gitlab/ci/config/entry/jobs_spec.rb

@@ -65,14 +65,12 @@ describe Gitlab::Ci::Config::Entry::Jobs do
         expect(entry.value).to eq(
           rspec: { name: :rspec,
                    script: %w[rspec],
-                   commands: 'rspec',
                    ignore: false,
                    stage: 'test',
                    only: { refs: %w[branches tags] },
                    except: {} },
           spinach: { name: :spinach,
                      script: %w[spinach],
-                     commands: 'spinach',
                      ignore: false,
                      stage: 'test',
                      only: { refs: %w[branches tags] },

spec/lib/gitlab/ci/pipeline/seed/build_spec.rb

@@ -6,8 +6,7 @@ describe Gitlab::Ci::Pipeline::Seed::Build do
 
   let(:attributes) do
     { name: 'rspec',
-      ref: 'master',
-      commands: 'rspec' }
+      ref: 'master' }
   end
 
   subject do
@@ -18,7 +17,7 @@ describe Gitlab::Ci::Pipeline::Seed::Build do
     it 'returns hash attributes of a build' do
       expect(subject.attributes).to be_a Hash
       expect(subject.attributes)
-        .to include(:name, :project, :ref, :commands)
+        .to include(:name, :project, :ref)
     end
   end
 

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -21,7 +21,6 @@ module Gitlab
               stage: "test",
               stage_idx: 1,
               name: "rspec",
-              commands: "pwd\nrspec",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -155,7 +154,6 @@ module Gitlab
              builds:
                [{ stage_idx: 1,
                   stage: "test",
-                  commands: "rspec",
                   tag_list: [],
                   name: "rspec",
                   allow_failure: false,
@@ -171,7 +169,6 @@ module Gitlab
              builds:
                [{ stage_idx: 2,
                   stage: "deploy",
-                  commands: "cap prod",
                   tag_list: [],
                   name: "prod",
                   allow_failure: false,
@@ -271,7 +268,7 @@ module Gitlab
             end
 
             it "return commands with scripts concencaced" do
-              expect(subject[:commands]).to eq("global script\nscript")
+              expect(subject[:options][:before_script]).to eq(["global script"])
             end
           end
 
@@ -284,7 +281,7 @@ module Gitlab
             end
 
             it "return commands with scripts concencaced" do
-              expect(subject[:commands]).to eq("local script\nscript")
+              expect(subject[:options][:before_script]).to eq(["local script"])
             end
           end
         end
@@ -297,7 +294,7 @@ module Gitlab
           end
 
           it "return commands with scripts concencaced" do
-            expect(subject[:commands]).to eq("script")
+            expect(subject[:options][:script]).to eq(["script"])
           end
         end
 
@@ -347,7 +344,6 @@ module Gitlab
               stage: "test",
               stage_idx: 1,
               name: "rspec",
-              commands: "pwd\nrspec",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -382,7 +378,6 @@ module Gitlab
               stage: "test",
               stage_idx: 1,
               name: "rspec",
-              commands: "pwd\nrspec",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -415,7 +410,6 @@ module Gitlab
               stage: "test",
               stage_idx: 1,
               name: "rspec",
-              commands: "pwd\nrspec",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -444,7 +438,6 @@ module Gitlab
               stage: "test",
               stage_idx: 1,
               name: "rspec",
-              commands: "pwd\nrspec",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -596,7 +589,7 @@ module Gitlab
 
           it 'correctly extends rspec job' do
             expect(config_processor.builds).to be_one
-            expect(subject.dig(:commands)).to eq 'test'
+            expect(subject.dig(:options, :script)).to eq %w(test)
             expect(subject.dig(:options, :image, :name)).to eq 'ruby:alpine'
           end
         end
@@ -622,7 +615,8 @@ module Gitlab
 
           it 'correctly extends rspec job' do
             expect(config_processor.builds).to be_one
-            expect(subject.dig(:commands)).to eq "bundle install\nrspec"
+            expect(subject.dig(:options, :before_script)).to eq ["bundle install"]
+            expect(subject.dig(:options, :script)).to eq %w(rspec)
             expect(subject.dig(:options, :image, :name)).to eq 'image:test'
             expect(subject.dig(:when)).to eq 'always'
           end
@@ -769,7 +763,6 @@ module Gitlab
             stage: "test",
             stage_idx: 1,
             name: "rspec",
-            commands: "pwd\nrspec",
             coverage_regex: nil,
             tag_list: [],
             options: {
@@ -983,7 +976,6 @@ module Gitlab
               stage: "test",
               stage_idx: 1,
               name: "normal_job",
-              commands: "test",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -1031,7 +1023,6 @@ module Gitlab
               stage: "build",
               stage_idx: 0,
               name: "job1",
-              commands: "execute-script-for-job",
               coverage_regex: nil,
               tag_list: [],
               options: {
@@ -1046,7 +1037,6 @@ module Gitlab
               stage: "build",
               stage_idx: 0,
               name: "job2",
-              commands: "execute-script-for-job",
               coverage_regex: nil,
               tag_list: [],
               options: {

spec/lib/gitlab/utils_spec.rb

@@ -197,4 +197,20 @@ describe Gitlab::Utils do
       end
     end
   end
+
+  describe '.deep_indifferent_access' do
+    let(:hash) do
+      { "variables" => [{ "key" => "VAR1", "value" => "VALUE2" }] }
+    end
+
+    subject { described_class.deep_indifferent_access(hash) }
+
+    it 'allows to access hash keys with symbols' do
+      expect(subject[:variables]).to be_a(Array)
+    end
+
+    it 'allows to access array keys with symbols' do
+      expect(subject[:variables].first[:key]).to eq('VAR1')
+    end
+  end
 end

spec/lib/serializers/json_spec.rb

+require 'fast_spec_helper'
+
+describe Serializers::JSON do
+  describe '.dump' do
+    let(:obj) { { key: "value" } }
+
+    subject { described_class.dump(obj) }
+
+    context 'when MySQL is used' do
+      before do
+        allow(Gitlab::Database).to receive(:adapter_name) { 'mysql2' }
+      end
+
+      it 'encodes as string' do
+        is_expected.to eq('{"key":"value"}')
+      end
+    end
+
+    context 'when PostgreSQL is used' do
+      before do
+        allow(Gitlab::Database).to receive(:adapter_name) { 'postgresql' }
+      end
+
+      it 'returns a hash' do
+        is_expected.to eq(obj)
+      end
+    end
+  end
+
+  describe '.load' do
+    let(:data_string) { '{"key":"value","variables":[{"key":"VAR1","value":"VALUE1"}]}' }
+    let(:data_hash) { JSON.parse(data_string) }
+
+    shared_examples 'having consistent accessor' do
+      it 'allows to access with symbols' do
+        expect(subject[:key]).to eq('value')
+        expect(subject[:variables].first[:key]).to eq('VAR1')
+      end
+
+      it 'allows to access with strings' do
+        expect(subject["key"]).to eq('value')
+        expect(subject["variables"].first["key"]).to eq('VAR1')
+      end
+    end
+
+    context 'when MySQL is used' do
+      before do
+        allow(Gitlab::Database).to receive(:adapter_name) { 'mysql2' }
+      end
+
+      context 'when loading a string' do
+        subject { described_class.load(data_string) }
+
+        it 'decodes a string' do
+          is_expected.to be_a(Hash)
+        end
+
+        it_behaves_like 'having consistent accessor'
+      end
+
+      context 'when loading a different type' do
+        subject { described_class.load({ key: 'hash' }) }
+
+        it 'raises an exception' do
+          expect { subject }.to raise_error(TypeError)
+        end
+      end
+
+      context 'when loading a nil' do
+        subject { described_class.load(nil) }
+
+        it 'returns nil' do
+          is_expected.to be_nil
+        end
+      end
+    end
+
+    context 'when PostgreSQL is used' do
+      before do
+        allow(Gitlab::Database).to receive(:adapter_name) { 'postgresql' }
+      end
+
+      context 'when loading a hash' do
+        subject { described_class.load(data_hash) }
+
+        it 'decodes a string' do
+          is_expected.to be_a(Hash)
+        end
+
+        it_behaves_like 'having consistent accessor'
+      end
+
+      context 'when loading a nil' do
+        subject { described_class.load(nil) }
+
+        it 'returns nil' do
+          is_expected.to be_nil
+        end
+      end
+    end
+  end
+end

spec/migrations/delete_inconsistent_internal_id_records_spec.rb

@@ -90,6 +90,13 @@ describe DeleteInconsistentInternalIdRecords, :migration do
 
   context 'for ci_pipelines' do
     let(:scope) { :ci_pipeline }
+
+    let(:create_models) do
+      create_list(:ci_empty_pipeline, 3, project: project1)
+      create_list(:ci_empty_pipeline, 3, project: project2)
+      create_list(:ci_empty_pipeline, 3, project: project3)
+    end
+
     it_behaves_like 'deleting inconsistent internal_id records'
   end
 

spec/models/ci/build_metadata_spec.rb

@@ -13,12 +13,12 @@ describe Ci::BuildMetadata do
   end
 
   let(:build) { create(:ci_build, pipeline: pipeline) }
-  let(:build_metadata) { build.metadata }
+  let(:metadata) { build.metadata }
 
   it_behaves_like 'having unique enum values'
 
   describe '#update_timeout_state' do
-    subject { build_metadata }
+    subject { metadata }
 
     context 'when runner is not assigned to the job' do
       it "doesn't change timeout value" do

spec/requests/api/runner_spec.rb

@@ -287,7 +287,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:job) do
       create(:ci_build, :artifacts, :extended_options,
-             pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, commands: "ls\ndate")
+             pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0)
     end
 
     describe 'POST /api/v4/jobs/request' do
@@ -422,7 +422,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
 
           let(:expected_steps) do
             [{ 'name' => 'script',
-               'script' => %w(ls date),
+               'script' => %w(echo),
                'timeout' => job.metadata_timeout,
                'when' => 'on_success',
                'allow_failure' => false },
@@ -588,7 +588,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             let!(:test_job) do
               create(:ci_build, pipeline: pipeline, token: 'test-job-token', name: 'deploy',
                                 stage: 'deploy', stage_idx: 1,
-                                options: { dependencies: [job2.name] })
+                                options: { script: ['bash'], dependencies: [job2.name] })
             end
 
             before do
@@ -612,7 +612,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             let!(:empty_dependencies_job) do
               create(:ci_build, pipeline: pipeline, token: 'test-job-token', name: 'empty_dependencies_job',
                                 stage: 'deploy', stage_idx: 1,
-                                options: { dependencies: [] })
+                                options: { script: ['bash'], dependencies: [] })
             end
 
             before do

spec/services/ci/process_pipeline_service_spec.rb

@@ -671,9 +671,9 @@ describe Ci::ProcessPipelineService, '#execute' do
 
   context 'when builds with auto-retries are configured' do
     before do
-      create_build('build:1', stage_idx: 0, user: user, options: { retry: { max: 2 } })
+      create_build('build:1', stage_idx: 0, user: user, options: { script: 'aa', retry: 2 })
       create_build('test:1', stage_idx: 1, user: user, when: :on_failure)
-      create_build('test:2', stage_idx: 1, user: user, options: { retry: { max: 1 } })
+      create_build('test:2', stage_idx: 1, user: user, options: { script: 'aa', retry: 1 })
     end
 
     it 'automatically retries builds in a valid order' do
@@ -770,7 +770,7 @@ describe Ci::ProcessPipelineService, '#execute' do
   end
 
   def delayed_options
-    { when: 'delayed', options: { start_in: '1 minute' } }
+    { when: 'delayed', options: { script: %w(echo), start_in: '1 minute' } }
   end
 
   def unschedule

spec/services/ci/register_job_service_spec.rb

@@ -460,7 +460,12 @@ module Ci
         end
 
         let!(:pre_stage_job) { create(:ci_build, :success, pipeline: pipeline, name: 'test', stage_idx: 0) }
-        let!(:pending_job) { create(:ci_build, :pending, pipeline: pipeline, stage_idx: 1, options: { dependencies: ['test'] } ) }
+
+        let!(:pending_job) do
+          create(:ci_build, :pending,
+            pipeline: pipeline, stage_idx: 1,
+            options: { script: ["bash"], dependencies: ['test'] })
+        end
 
         subject { execute(specific_runner) }