Merge branch 'ci-scope-metadata-config-flag-to-projects' into 'master'

Scope ci_build_metadata_config feature flag to projects

See merge request gitlab-org/gitlab!64327

app/finders/security/jobs_finder.rb

@@ -38,7 +38,7 @@ module Security
     def execute
       return [] if @job_types.empty?
 
-      if Feature.enabled?(:ci_build_metadata_config)
+      if Feature.enabled?(:ci_build_metadata_config, pipeline.project, default_enabled: :yaml)
         find_jobs
       else
         find_jobs_legacy

app/models/ci/build_metadata.rb

@@ -22,8 +22,8 @@ module Ci
     validates :build, presence: true
     validates :secrets, json_schema: { filename: 'build_metadata_secrets' }
 
-    serialize :config_options, Serializers::Json # rubocop:disable Cop/ActiveRecordSerialize
-    serialize :config_variables, Serializers::Json # rubocop:disable Cop/ActiveRecordSerialize
+    serialize :config_options, Serializers::SymbolizedJson # rubocop:disable Cop/ActiveRecordSerialize
+    serialize :config_variables, Serializers::SymbolizedJson # rubocop:disable Cop/ActiveRecordSerialize
 
     chronic_duration_attr_reader :timeout_human_readable, :timeout
 

app/models/concerns/ci/metadatable.rb

@@ -77,7 +77,7 @@ module Ci
 
     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)
+      if Feature.enabled?(:ci_build_metadata_config, project, default_enabled: :yaml)
         ensure_metadata.write_attribute(metadata_key, value)
         write_attribute(legacy_key, nil)
       else

config/feature_flags/development/ci_build_metadata_config.yml

 ---
 name: ci_build_metadata_config
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/7238
-rollout_issue_url: 
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330954
 milestone: '11.7'
 type: development
 group: group::pipeline execution

ee/app/models/concerns/latest_pipeline_information.rb

@@ -11,13 +11,9 @@ module LatestPipelineInformation
     strong_memoize("latest_builds_reports_#{only_successful_builds}" ) do
       builds = latest_security_builds
       builds = builds.select { |build| build.status == 'success' } if only_successful_builds
-      builds.map do |build|
-        if Feature.enabled?(:ci_build_metadata_config)
-          build.metadata.config_options[:artifacts][:reports].keys.map(&:to_sym)
-        else
-          build.options[:artifacts][:reports].keys
-        end
-      end.flatten
+      builds.flat_map do |build|
+        build.options[:artifacts][:reports].keys
+      end
     end
   end
 

ee/spec/presenters/projects/security/configuration_presenter_spec.rb

@@ -167,8 +167,7 @@ RSpec.describe Projects::Security::ConfigurationPresenter do
 
       it 'detects security jobs even when the job has more than one report' do
         config = { artifacts: { reports: { other_job: ['gl-other-report.json'], sast: ['gl-sast-report.json'] } } }
-        complicated_metadata = double(:complicated_metadata, config_options: config)
-        complicated_job = double(:complicated_job, metadata: complicated_metadata)
+        complicated_job = build_stubbed(:ci_build, options: config)
 
         allow_next_instance_of(::Security::SecurityJobsFinder) do |finder|
           allow(finder).to receive(:execute).and_return([complicated_job])

ee/spec/services/ci/create_pipeline_service/cross_needs_artifacts_spec.rb

@@ -47,7 +47,7 @@ RSpec.describe Ci::CreatePipelineService do
     end
 
     it 'persists cross_dependencies' do
-      deps = build_job.options['cross_dependencies']
+      deps = build_job.options[:cross_dependencies]
       result = [
         { job: "job-1", ref: "ref-1", project: "project-1", artifacts: true },
         { job: "job-2", ref: "ref-2", project: "project-2", artifacts: false },
@@ -142,7 +142,7 @@ RSpec.describe Ci::CreatePipelineService do
     end
 
     it 'persists cross_dependencies' do
-      deps = test_job.options['cross_dependencies']
+      deps = test_job.options[:cross_dependencies]
       result = {
         job: 'dependency',
         ref: 'master',

ee/spec/services/ci/create_pipeline_service_spec.rb

@@ -96,7 +96,7 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
       expect(bridge).to be_a Ci::Bridge
       expect(bridge.stage).to eq 'deploy'
       expect(pipeline.statuses).to match_array [test, bridge]
-      expect(bridge.options).to eq('trigger' => { 'project' => 'my/project' })
+      expect(bridge.options).to eq(trigger: { project: 'my/project' })
       expect(bridge.yaml_variables)
         .to include(key: 'CROSS', value: 'downstream', public: true)
     end

ee/spec/services/ci/run_dast_scan_service_spec.rb

@@ -83,15 +83,15 @@ RSpec.describe Ci::RunDastScanService do
       it 'creates a build with appropriate options' do
         build = pipeline.builds.first
         expected_options = {
-          'image' => {
-            'name' => '$SECURE_ANALYZERS_PREFIX/dast:$DAST_VERSION'
+          image: {
+            name: '$SECURE_ANALYZERS_PREFIX/dast:$DAST_VERSION'
           },
-          'script' => [
+          script: [
             '/analyze'
           ],
-          'artifacts' => {
-            'reports' => {
-              'dast' => ['gl-dast-report.json']
+          artifacts: {
+            reports: {
+              dast: ['gl-dast-report.json']
             }
           }
         }
@@ -103,61 +103,61 @@ RSpec.describe Ci::RunDastScanService do
 
         expected_variables = [
           {
-            'key' => 'DAST_AUTH_URL',
-            'value' => dast_site_profile.auth_url,
-            'public' => true
+            key: 'DAST_AUTH_URL',
+            value: dast_site_profile.auth_url,
+            public: true
           }, {
-            'key' => 'DAST_DEBUG',
-            'value' => String(dast_scanner_profile.show_debug_messages?),
-            'public' => true
+            key: 'DAST_DEBUG',
+            value: String(dast_scanner_profile.show_debug_messages?),
+            public: true
           }, {
-            'key' => 'DAST_EXCLUDE_URLS',
-            'value' => dast_site_profile.excluded_urls.join(','),
-            'public' => true
+            key: 'DAST_EXCLUDE_URLS',
+            value: dast_site_profile.excluded_urls.join(','),
+            public: true
           }, {
-            'key' => 'DAST_FULL_SCAN_ENABLED',
-            'value' => String(dast_scanner_profile.full_scan_enabled?),
-            'public' => true
+            key: 'DAST_FULL_SCAN_ENABLED',
+            value: String(dast_scanner_profile.full_scan_enabled?),
+            public: true
           }, {
-            'key' => 'DAST_PASSWORD_FIELD',
-            'value' => dast_site_profile.auth_password_field,
-            'public' => true
+            key: 'DAST_PASSWORD_FIELD',
+            value: dast_site_profile.auth_password_field,
+            public: true
           }, {
-            'key' => 'DAST_SPIDER_MINS',
-            'value' => String(dast_scanner_profile.spider_timeout),
-            'public' => true
+            key: 'DAST_SPIDER_MINS',
+            value: String(dast_scanner_profile.spider_timeout),
+            public: true
           }, {
-            'key' => 'DAST_TARGET_AVAILABILITY_TIMEOUT',
-            'value' => String(dast_scanner_profile.target_timeout),
-            'public' => true
+            key: 'DAST_TARGET_AVAILABILITY_TIMEOUT',
+            value: String(dast_scanner_profile.target_timeout),
+            public: true
           }, {
-            'key' => 'DAST_USERNAME',
-            'value' => dast_site_profile.auth_username,
-            'public' => true
+            key: 'DAST_USERNAME',
+            value: dast_site_profile.auth_username,
+            public: true
           }, {
-            'key' => 'DAST_USERNAME_FIELD',
-            'value' => dast_site_profile.auth_username_field,
-            'public' => true
+            key: 'DAST_USERNAME_FIELD',
+            value: dast_site_profile.auth_username_field,
+            public: true
           }, {
-            'key' => 'DAST_USE_AJAX_SPIDER',
-            'value' => String(dast_scanner_profile.use_ajax_spider?),
-            'public' => true
+            key: 'DAST_USE_AJAX_SPIDER',
+            value: String(dast_scanner_profile.use_ajax_spider?),
+            public: true
           }, {
-            'key' => 'DAST_VERSION',
-            'value' => '1',
-            'public' => true
+            key: 'DAST_VERSION',
+            value: '1',
+            public: true
           }, {
-            'key' => 'DAST_WEBSITE',
-            'value' => dast_site_profile.dast_site.url,
-            'public' => true
+            key: 'DAST_WEBSITE',
+            value: dast_site_profile.dast_site.url,
+            public: true
           }, {
-            'key' => 'GIT_STRATEGY',
-            'value' => 'none',
-            'public' => true
+            key: 'GIT_STRATEGY',
+            value: 'none',
+            public: true
           }, {
-            'key' => 'SECURE_ANALYZERS_PREFIX',
-            'value' => 'registry.gitlab.com/gitlab-org/security-products/analyzers',
-            'public' => true
+            key: 'SECURE_ANALYZERS_PREFIX',
+            value: 'registry.gitlab.com/gitlab-org/security-products/analyzers',
+            public: true
           }
         ]
 

lib/gitlab/utils.rb

@@ -169,6 +169,16 @@ module Gitlab
       end
     end
 
+    def deep_symbolized_access(data)
+      if data.is_a?(Array)
+        data.map(&method(:deep_symbolized_access))
+      elsif data.is_a?(Hash)
+        data.deep_symbolize_keys
+      else
+        data
+      end
+    end
+
     def string_to_ip_object(str)
       return unless str
 

lib/serializers/symbolized_json.rb

+# frozen_string_literal: true
+
+module Serializers
+  # Make the resulting hash have deep symbolized keys
+  class SymbolizedJson
+    class << self
+      def dump(obj)
+        obj
+      end
+
+      def load(data)
+        return if data.nil?
+
+        Gitlab::Utils.deep_symbolized_access(data)
+      end
+    end
+  end
+end

spec/lib/gitlab/utils_spec.rb

@@ -351,6 +351,22 @@ RSpec.describe Gitlab::Utils do
     end
   end
 
+  describe '.deep_symbolized_access' do
+    let(:hash) do
+      { "variables" => [{ "key" => "VAR1", "value" => "VALUE2" }] }
+    end
+
+    subject { described_class.deep_symbolized_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
+
   describe '.try_megabytes_to_bytes' do
     context 'when the size can be converted to megabytes' do
       it 'returns the size in megabytes' do

spec/lib/serializers/symbolized_json_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+RSpec.describe Serializers::SymbolizedJson do
+  describe '.dump' do
+    let(:obj) { { key: "value" } }
+
+    subject { described_class.dump(obj) }
+
+    it 'returns a hash' do
+      is_expected.to eq(obj)
+    end
+  end
+
+  describe '.load' do
+    let(:data_string) { '{"key":"value","variables":[{"key":"VAR1","value":"VALUE1"}]}' }
+    let(:data_hash) { Gitlab::Json.parse(data_string) }
+
+    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 'allows to access with symbols' do
+        expect(subject[:key]).to eq('value')
+        expect(subject[:variables].first[:key]).to eq('VAR1')
+      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
+end

spec/models/ci/build_spec.rb

@@ -2172,15 +2172,15 @@ RSpec.describe Ci::Build do
     end
 
     it 'contains options' do
-      expect(build.options).to eq(options.stringify_keys)
+      expect(build.options).to eq(options.symbolize_keys)
     end
 
-    it 'allows to access with keys' do
+    it 'allows to access with symbolized keys' do
       expect(build.options[:image]).to eq('ruby:2.7')
     end
 
-    it 'allows to access with strings' do
-      expect(build.options['image']).to eq('ruby:2.7')
+    it 'rejects access with string keys' do
+      expect(build.options['image']).to be_nil
     end
 
     context 'when ci_build_metadata_config is set' do
@@ -2189,7 +2189,7 @@ RSpec.describe Ci::Build do
       end
 
       it 'persist data in build metadata' do
-        expect(build.metadata.read_attribute(:config_options)).to eq(options.stringify_keys)
+        expect(build.metadata.read_attribute(:config_options)).to eq(options.symbolize_keys)
       end
 
       it 'does not persist data in build' do
@@ -4715,9 +4715,9 @@ RSpec.describe Ci::Build do
 
   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" } }
+    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) }
 
@@ -4752,8 +4752,8 @@ RSpec.describe Ci::Build do
 
   describe '#write_metadata_attribute' do
     let(:build) { create(:ci_build, :degenerated) }
-    let(:options) { { "key" => "new options" } }
-    let(:existing_options) { { "key" => "existing options" } }
+    let(:options) { { key: "new options" } }
+    let(:existing_options) { { key: "existing options" } }
 
     subject { build.send(:write_metadata_attribute, :options, :config_options, options) }
 

spec/services/ci/create_pipeline_service/cache_spec.rb

@@ -33,11 +33,11 @@ RSpec.describe Ci::CreatePipelineService do
 
       it 'uses the provided key' do
         expected = {
-          'key'       => 'a-key',
-          'paths'     => ['logs/', 'binaries/'],
-          'policy'    => 'pull-push',
-          'untracked' => true,
-          'when'      => 'on_success'
+          key: 'a-key',
+          paths: ['logs/', 'binaries/'],
+          policy: 'pull-push',
+          untracked: true,
+          when: 'on_success'
         }
 
         expect(pipeline).to be_persisted
@@ -66,10 +66,10 @@ RSpec.describe Ci::CreatePipelineService do
 
         it 'builds a cache key' do
           expected = {
-            'key'       => /[a-f0-9]{40}/,
-            'paths'     => ['logs/'],
-            'policy'    => 'pull-push',
-            'when'      => 'on_success'
+            key: /[a-f0-9]{40}/,
+            paths: ['logs/'],
+            policy: 'pull-push',
+            when: 'on_success'
           }
 
           expect(pipeline).to be_persisted
@@ -82,10 +82,10 @@ RSpec.describe Ci::CreatePipelineService do
 
         it 'uses default cache key' do
           expected = {
-            'key'       => /default/,
-            'paths'     => ['logs/'],
-            'policy'    => 'pull-push',
-            'when'      => 'on_success'
+            key: /default/,
+            paths: ['logs/'],
+            policy: 'pull-push',
+            when: 'on_success'
           }
 
           expect(pipeline).to be_persisted
@@ -115,10 +115,10 @@ RSpec.describe Ci::CreatePipelineService do
 
         it 'builds a cache key' do
           expected = {
-            'key'       => /\$ENV_VAR-[a-f0-9]{40}/,
-            'paths'     => ['logs/'],
-            'policy'    => 'pull-push',
-            'when'      => 'on_success'
+            key: /\$ENV_VAR-[a-f0-9]{40}/,
+            paths: ['logs/'],
+            policy: 'pull-push',
+            when: 'on_success'
           }
 
           expect(pipeline).to be_persisted
@@ -131,10 +131,10 @@ RSpec.describe Ci::CreatePipelineService do
 
         it 'uses default cache key' do
           expected = {
-            'key'       => /\$ENV_VAR-default/,
-            'paths'     => ['logs/'],
-            'policy'    => 'pull-push',
-            'when'      => 'on_success'
+            key: /\$ENV_VAR-default/,
+            paths: ['logs/'],
+            policy: 'pull-push',
+            when: 'on_success'
           }
 
           expect(pipeline).to be_persisted

spec/services/ci/create_pipeline_service/custom_yaml_tags_spec.rb

@@ -39,8 +39,8 @@ RSpec.describe Ci::CreatePipelineService do
       it 'creates a pipeline' do
         expect(pipeline).to be_persisted
         expect(pipeline.builds.first.options).to match(a_hash_including({
-          'before_script' => ['ls'],
-          'script' => [
+          before_script: ['ls'],
+          script: [
             'echo doing my first step',
             'echo doing step 1 of job 1',
             'echo doing my last step'

spec/services/ci/create_pipeline_service/needs_spec.rb

@@ -104,7 +104,7 @@ RSpec.describe Ci::CreatePipelineService do
 
       it 'saves dependencies' do
         expect(test_a_build.options)
-          .to match(a_hash_including('dependencies' => ['build_a']))
+          .to match(a_hash_including(dependencies: ['build_a']))
       end
 
       it 'artifacts default to true' do

spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb

@@ -69,9 +69,9 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
     it_behaves_like 'successful creation' do
       let(:expected_bridge_options) do
         {
-          'trigger' => {
-            'include' => [
-              { 'local' => 'path/to/child.yml' }
+          trigger: {
+            include: [
+              { local: 'path/to/child.yml' }
             ]
           }
         }
@@ -149,9 +149,9 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
       it_behaves_like 'successful creation' do
         let(:expected_bridge_options) do
           {
-            'trigger' => {
-              'include' => [
-                { 'local' => 'path/to/child.yml' }
+            trigger: {
+              include: [
+                { local: 'path/to/child.yml' }
               ]
             }
           }
@@ -175,8 +175,8 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
         it_behaves_like 'successful creation' do
           let(:expected_bridge_options) do
             {
-              'trigger' => {
-                'include' => 'path/to/child.yml'
+              trigger: {
+                include: 'path/to/child.yml'
               }
             }
           end
@@ -202,8 +202,8 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
         it_behaves_like 'successful creation' do
           let(:expected_bridge_options) do
             {
-              'trigger' => {
-                'include' => ['path/to/child.yml', 'path/to/child2.yml']
+              trigger: {
+                include: ['path/to/child.yml', 'path/to/child2.yml']
               }
             }
           end
@@ -295,12 +295,12 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
         it_behaves_like 'successful creation' do
           let(:expected_bridge_options) do
             {
-              'trigger' => {
-                'include' => [
+              trigger: {
+                include: [
                   {
-                    'file' => 'path/to/child.yml',
-                    'project' => 'my-namespace/my-project',
-                    'ref' => 'master'
+                    file: 'path/to/child.yml',
+                    project: 'my-namespace/my-project',
+                    ref: 'master'
                   }
                 ]
               }
@@ -353,11 +353,11 @@ RSpec.describe Ci::CreatePipelineService, '#execute' do
         it_behaves_like 'successful creation' do
           let(:expected_bridge_options) do
             {
-              'trigger' => {
-                'include' => [
+              trigger: {
+                include: [
                   {
-                    'file' => ["path/to/child1.yml", "path/to/child2.yml"],
-                    'project' => 'my-namespace/my-project'
+                    file: ["path/to/child1.yml", "path/to/child2.yml"],
+                    project: 'my-namespace/my-project'
                   }
                 ]
               }

spec/services/ci/create_pipeline_service_spec.rb

@@ -1001,7 +1001,7 @@ RSpec.describe Ci::CreatePipelineService do
           expect(pipeline.yaml_errors).not_to be_present
           expect(pipeline).to be_persisted
           expect(build).to be_kind_of(Ci::Build)
-          expect(build.options).to eq(config[:release].except(:stage, :only).with_indifferent_access)
+          expect(build.options).to eq(config[:release].except(:stage, :only))
           expect(build).to be_persisted
         end
       end