Add config_options|variables to BuildMetadata

These are data columns that store runtime configuration
of build needed to execute it on runner and within pipeline.

The definition of this data is that once used, and when no longer
needed (due to retry capability) they can be freely removed.

They use `jsonb` on PostgreSQL, and `text` on MySQL (due to lacking
support for json datatype on old enough version).

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/models/ci/build_spec.rb

@@ -1457,8 +1457,24 @@ describe Ci::Build do
       context 'with retries max config option' do
         subject { create(:ci_build, options: { retry: { max: 1 } }) }
 
-        it 'returns the number of configured max retries' do
-          expect(subject.retries_max).to eq 1
+        context 'when build_metadata_config is set' do
+          before do
+            stub_feature_flags(ci_build_metadata_config: true)
+          end
+
+          it 'returns the number of configured max retries' do
+            expect(subject.retries_max).to eq 1
+          end
+        end
+
+        context 'when build_metadata_config is not set' do
+          before do
+            stub_feature_flags(ci_build_metadata_config: false)
+          end
+
+          it 'returns the number of configured max retries' do
+            expect(subject.retries_max).to eq 1
+          end
         end
       end
 
@@ -1679,14 +1695,49 @@ describe Ci::Build do
     let(:options) do
       {
         image: "ruby:2.1",
-        services: [
-          "postgres"
-        ]
+        services: ["postgres"],
+        script: ["ls -a"]
       }
     end
 
     it 'contains options' do
-      expect(build.options).to eq(options)
+      expect(build.options).to eq(options.stringify_keys)
+    end
+
+    it 'allows to access with keys' do
+      expect(build.options[:image]).to eq('ruby:2.1')
+    end
+
+    it 'allows to access with strings' do
+      expect(build.options['image']).to eq('ruby:2.1')
+    end
+
+    context 'when ci_build_metadata_config is set' do
+      before do
+        stub_feature_flags(ci_build_metadata_config: true)
+      end
+
+      it 'persist data in build metadata' do
+        expect(build.metadata.read_attribute(:config_options)).to eq(options.stringify_keys)
+      end
+
+      it 'does not persist data in build' do
+        expect(build.read_attribute(:options)).to be_nil
+      end
+    end
+
+    context 'when ci_build_metadata_config is disabled' do
+      before do
+        stub_feature_flags(ci_build_metadata_config: false)
+      end
+
+      it 'persist data in build' do
+        expect(build.read_attribute(:options)).to eq(options.symbolize_keys)
+      end
+
+      it 'does not persist data in build metadata' do
+        expect(build.metadata.read_attribute(:config_options)).to be_nil
+      end
     end
   end
 
@@ -2030,56 +2081,6 @@ describe Ci::Build do
     end
   end
 
-  describe '#when' do
-    subject { build.when }
-
-    context 'when `when` is undefined' do
-      before do
-        build.when = nil
-      end
-
-      context 'use from gitlab-ci.yml' do
-        let(:project) { create(:project, :repository) }
-        let(:pipeline) { create(:ci_pipeline, project: project) }
-
-        before do
-          stub_ci_pipeline_yaml_file(config)
-        end
-
-        context 'when config is not found' do
-          let(:config) { nil }
-
-          it { is_expected.to eq('on_success') }
-        end
-
-        context 'when 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 'when 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 '#variables' do
     let(:container_registry_enabled) { false }
 
@@ -2148,62 +2149,6 @@ describe Ci::Build do
 
       it { is_expected.to include(*predefined_variables) }
 
-      context 'when yaml variables are undefined' do
-        let(:pipeline) do
-          create(:ci_pipeline, project: project,
-                               sha: project.commit.id,
-                               ref: project.default_branch)
-        end
-
-        before do
-          build.yaml_variables = nil
-        end
-
-        context 'use from gitlab-ci.yml' do
-          before do
-            stub_ci_pipeline_yaml_file(config)
-          end
-
-          context 'when config is not found' do
-            let(:config) { nil }
-
-            it { is_expected.to include(*predefined_variables) }
-          end
-
-          context 'when config does not have a questioned job' do
-            let(:config) do
-              YAML.dump({
-                test_other: {
-                  script: 'Hello World'
-                }
-              })
-            end
-
-            it { is_expected.to include(*predefined_variables) }
-          end
-
-          context 'when 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 include(*predefined_variables) }
-            it { is_expected.to include(*variables) }
-          end
-        end
-      end
-
       describe 'variables ordering' do
         context 'when variables hierarchy is stubbed' do
           let(:build_pre_var) { { key: 'build', value: 'value', public: true } }
@@ -2792,29 +2737,53 @@ describe Ci::Build do
   end
 
   describe '#yaml_variables' do
-    before do
-      build.update_attribute(:yaml_variables, variables)
+    let(:build) { create(:ci_build, pipeline: pipeline, yaml_variables: variables) }
+
+    let(:variables) do
+      [
+        { 'key' => :VARIABLE, 'value' => 'my value' },
+        { 'key' => 'VARIABLE2', 'value' => 'my value 2' }
+      ]
     end
 
-    context 'when serialized valu is a symbolized hash' do
-      let(:variables) do
-        [{ key: :VARIABLE, value: 'my value 1' }]
+    shared_examples 'having consistent representation' do
+      it 'allows to access using symbols' do
+        expect(build.reload.yaml_variables.first[:key]).to eq('VARIABLE')
+        expect(build.reload.yaml_variables.first[:value]).to eq('my value')
+        expect(build.reload.yaml_variables.second[:key]).to eq('VARIABLE2')
+        expect(build.reload.yaml_variables.second[:value]).to eq('my value 2')
       end
+    end
+
+    context 'when ci_build_metadata_config is set' do
+      before do
+        stub_feature_flags(ci_build_metadata_config: true)
+      end
+
+      it_behaves_like 'having consistent representation'
 
-      it 'keeps symbolizes keys and stringifies variables names' do
-        expect(build.yaml_variables)
-          .to eq [{ key: 'VARIABLE', value: 'my value 1' }]
+      it 'persist data in build metadata' do
+        expect(build.metadata.read_attribute(:config_variables)).not_to be_nil
+      end
+
+      it 'does not persist data in build' do
+        expect(build.read_attribute(:yaml_variables)).to be_nil
       end
     end
 
-    context 'when serialized value is a hash with string keys' do
-      let(:variables) do
-        [{ 'key' => :VARIABLE, 'value' => 'my value 2' }]
+    context 'when ci_build_metadata_config is disabled' do
+      before do
+        stub_feature_flags(ci_build_metadata_config: false)
       end
 
-      it 'symblizes variables hash' do
-        expect(build.yaml_variables)
-          .to eq [{ key: 'VARIABLE', value: 'my value 2' }]
+      it_behaves_like 'having consistent representation'
+
+      it 'persist data in build' do
+        expect(build.read_attribute(:yaml_variables)).not_to be_nil
+      end
+
+      it 'does not persist data in build metadata' do
+        expect(build.metadata.read_attribute(:config_variables)).to be_nil
       end
     end
   end
@@ -2986,7 +2955,7 @@ describe Ci::Build do
     end
 
     context 'when build is configured to be retried' do
-      subject { create(:ci_build, :running, options: { retry: { max: 3 } }, project: project, user: user) }
+      subject { create(:ci_build, :running, options: { script: ["ls -al"], retry: 3 }, project: project, user: user) }
 
       it 'retries build and assigns the same user to it' do
         expect(described_class).to receive(:retry)
@@ -3475,6 +3444,23 @@ describe Ci::Build do
     end
   end
 
+  describe 'degenerate!' do
+    let(:build) { create(:ci_build) }
+
+    subject { build.degenerate! }
+
+    before do
+      build.ensure_metadata
+    end
+
+    it 'drops metadata' do
+      subject
+
+      expect(build.reload).to be_degenerated
+      expect(build.metadata).to be_nil
+    end
+  end
+
   describe '#archived?' do
     context 'when build is degenerated' do
       subject { create(:ci_build, :degenerated) }
@@ -3502,4 +3488,97 @@ describe Ci::Build do
       end
     end
   end
+
+  describe '#read_metadata_attribute' do
+    let(:build) { create(:ci_build, :degenerated) }
+    let(:build_options) { { "key" => "build" } }
+    let(:metadata_options) { { "key" => "metadata" } }
+    let(:default_options) { { "key" => "default" } }
+
+    subject { build.send(:read_metadata_attribute, :options, :config_options, default_options) }
+
+    context 'when build and metadata options is set' do
+      before do
+        build.write_attribute(:options, build_options)
+        build.ensure_metadata.write_attribute(:config_options, metadata_options)
+      end
+
+      it 'prefers build options' do
+        is_expected.to eq(build_options)
+      end
+    end
+
+    context 'when only metadata options is set' do
+      before do
+        build.write_attribute(:options, nil)
+        build.ensure_metadata.write_attribute(:config_options, metadata_options)
+      end
+
+      it 'returns metadata options' do
+        is_expected.to eq(metadata_options)
+      end
+    end
+
+    context 'when none is set' do
+      it 'returns default value' do
+        is_expected.to eq(default_options)
+      end
+    end
+  end
+
+  describe '#write_metadata_attribute' do
+    let(:build) { create(:ci_build, :degenerated) }
+    let(:options) { { "key" => "new options" } }
+    let(:existing_options) { { "key" => "existing options" } }
+
+    subject { build.send(:write_metadata_attribute, :options, :config_options, options) }
+
+    context 'when ci_build_metadata_config is set' do
+      before do
+        stub_feature_flags(ci_build_metadata_config: true)
+      end
+
+      context 'when data in build is already set' do
+        before do
+          build.write_attribute(:options, existing_options)
+        end
+
+        it 'does set metadata options' do
+          subject
+
+          expect(build.metadata.read_attribute(:config_options)).to eq(options)
+        end
+
+        it 'does reset build options' do
+          subject
+
+          expect(build.read_attribute(:options)).to be_nil
+        end
+      end
+    end
+
+    context 'when ci_build_metadata_config is disabled' do
+      before do
+        stub_feature_flags(ci_build_metadata_config: false)
+      end
+
+      context 'when data in build metadata is already set' do
+        before do
+          build.ensure_metadata.write_attribute(:config_options, existing_options)
+        end
+
+        it 'does set metadata options' do
+          subject
+
+          expect(build.read_attribute(:options)).to eq(options)
+        end
+
+        it 'does reset build options' do
+          subject
+
+          expect(build.metadata.read_attribute(:config_options)).to be_nil
+        end
+      end
+    end
+  end
 end

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) }