From 5dfb071f2898ebf9fdfe980b56c120a82240ddf4 Mon Sep 17 00:00:00 2001
From: Laura Montemayor <lmontemayor@gitlab.com>
Date: Mon, 24 Mar 2025 15:01:21 +0100
Subject: [PATCH 1/2] Extract CI logic from BaseHooksService to
 Ci::PipelineCreation::PushOptions

---
 app/services/ci/create_pipeline_service.rb    |   2 +-
 app/services/git/base_hooks_service.rb        |  49 ++------
 .../merge_requests/create_pipeline_service.rb |   2 +-
 .../merge_requests/create_pipeline_service.rb |   2 +-
 lib/ci/pipeline_creation/push_options.rb      |  55 ++++++++
 lib/gitlab/ci/pipeline/chain/skip.rb          |   7 +-
 .../ci/pipeline_creation/push_options_spec.rb | 118 ++++++++++++++++++
 .../ci/create_pipeline_service_spec.rb        |   8 +-
 spec/services/git/base_hooks_service_spec.rb  |  15 +--
 spec/services/git/branch_push_service_spec.rb |   2 +-
 10 files changed, 195 insertions(+), 65 deletions(-)
 create mode 100644 lib/ci/pipeline_creation/push_options.rb
 create mode 100644 spec/lib/ci/pipeline_creation/push_options_spec.rb

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index a8a4156319181c..07873ac27fca4b 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -93,7 +93,7 @@ def execute(
         variables_attributes: params[:variables_attributes],
         project: project,
         current_user: current_user,
-        push_options: params[:push_options] || {},
+        push_options: params[:push_options] || ::Ci::PipelineCreation::PushOptions.new({}),
         chat_data: params[:chat_data],
         bridge: bridge,
         logger: @logger,
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index 8144aec9e96d58..957cc4d0ff34d7 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -98,17 +98,17 @@ def pipeline_params
           before: oldrev,
           after: newrev,
           ref: ref,
-          variables_attributes: generate_vars_from_push_options || [],
-          push_options: params[:push_options] || {},
+          variables_attributes: ci_push_options.variables,
+          push_options: ci_push_options,
           checkout_sha: Gitlab::DataBuilder::Push.checkout_sha(
             project.repository, newrev, ref)
         }
       end
     end
 
-    def ci_variables_from_push_options
-      strong_memoize(:ci_variables_from_push_options) do
-        push_options&.dig(:ci, :variable)
+    def ci_push_options
+      strong_memoize(:ci_push_options) do
+        Ci::PipelineCreation::PushOptions.new(push_options)
       end
     end
 
@@ -120,27 +120,10 @@ def integration_push_options
 
     def push_options
       strong_memoize(:push_options) do
-        params[:push_options]&.deep_symbolize_keys
+        params[:push_options] || {}
       end
     end
 
-    def generate_vars_from_push_options
-      return [] unless ci_variables_from_push_options
-
-      ci_variables_from_push_options.map do |var_definition, _count|
-        key, value = var_definition.to_s.split("=", 2)
-
-        # Accept only valid format. We ignore the following formats
-        # 1. "=123". In this case, `key` will be an empty string
-        # 2. "FOO". In this case, `value` will be nil.
-        # However, the format "FOO=" will result in key being `FOO` and value
-        # being an empty string. This is acceptable.
-        next if key.blank? || value.nil?
-
-        { "key" => key, "variable_type" => "env_var", "secret_value" => value }
-      end.compact
-    end
-
     def push_data_params(commits:, with_changed_files: true)
       {
         oldrev: oldrev,
@@ -172,29 +155,11 @@ def push_data
 
     # merges with EE override
     def pipeline_options
-      return {} unless ci_inputs_from_push_options
-
       {
-        inputs: generate_ci_inputs_from_push_options || {}
+        inputs: ci_push_options.inputs
       }
     end
 
-    def ci_inputs_from_push_options
-      strong_memoize(:ci_inputs_from_push_options) do
-        push_options&.dig(:ci, :input)
-      end
-    end
-
-    def generate_ci_inputs_from_push_options
-      return {} unless ci_inputs_from_push_options
-
-      params = ci_inputs_from_push_options.map do |input, _|
-        input.to_s.split("=", 2)
-      end
-
-      ::Ci::PipelineCreation::Inputs.parse_params(params.to_h)
-    end
-
     def log_pipeline_errors(error_message)
       data = {
         class: self.class.name,
diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb
index 07390b4a6685aa..6f38ea0f981bb1 100644
--- a/app/services/merge_requests/create_pipeline_service.rb
+++ b/app/services/merge_requests/create_pipeline_service.rb
@@ -27,7 +27,7 @@ def create_detached_merge_request_pipeline(merge_request)
       Ci::CreatePipelineService.new(project,
         current_user,
         ref: ref,
-        push_options: params[:push_options],
+        push_options: ::Ci::PipelineCreation::PushOptions.new(params[:push_options]),
         pipeline_creation_request: params[:pipeline_creation_request]
       ).execute(:merge_request_event, merge_request: merge_request)
     end
diff --git a/ee/app/services/ee/merge_requests/create_pipeline_service.rb b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
index 06c7905dfb8290..3b2b3568123eeb 100644
--- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb
+++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
@@ -29,7 +29,7 @@ def create_merged_result_pipeline_for(merge_request)
               target_sha: ref_payload[:target_id],
               source_sha: ref_payload[:source_id],
               pipeline_creation_request: params[:pipeline_creation_request],
-              push_options: params[:push_options])
+              push_options: ::Ci::PipelineCreation::PushOptions.new(params[:push_options]))
             .execute(:merge_request_event, merge_request: merge_request)
         else
           cannot_create_pipeline_error
diff --git a/lib/ci/pipeline_creation/push_options.rb b/lib/ci/pipeline_creation/push_options.rb
new file mode 100644
index 00000000000000..08f9bb52b6b5de
--- /dev/null
+++ b/lib/ci/pipeline_creation/push_options.rb
@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+module Ci
+  module PipelineCreation
+    class PushOptions
+      def initialize(push_options)
+        @push_options = push_options&.deep_symbolize_keys || {}
+      end
+
+      def skips_ci?
+        push_options.dig(:ci, :skip).present?
+      end
+
+      def variables
+        raw_push_options_variables = push_options.dig(:ci, :variable)
+        return [] unless raw_push_options_variables
+
+        raw_vars = extract_key_value_pairs_from_push_option(raw_push_options_variables)
+
+        raw_vars.map do |key, value|
+          { "key" => key, "variable_type" => "env_var", "secret_value" => value }
+        end
+      end
+
+      def inputs
+        raw_push_options_inputs = push_options.dig(:ci, :input)
+        return {} unless raw_push_options_inputs
+
+        raw_inputs = extract_key_value_pairs_from_push_option(raw_push_options_inputs)
+        ::Ci::PipelineCreation::Inputs.parse_params(raw_inputs.to_h)
+      end
+
+      private
+
+      attr_reader :push_options
+
+      def extract_key_value_pairs_from_push_option(push_option)
+        # When extracting variables and encountering a missing `key` or `value`, this is valid:
+        #  "ABC=" -> `key` would be `ABC` and value an empty string
+        # These formats are invalid and will be ignored:
+        #  "=123" -> `key` would be an empty string
+        #  "ABC"  -> `value` would be nil
+
+        return [] unless push_option
+
+        push_option.each_with_object([]) do |(raw_value, _), result|
+          key, value = raw_value.to_s.split("=", 2)
+          next if key.blank? || value.nil?
+
+          result << [key, value]
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/chain/skip.rb b/lib/gitlab/ci/pipeline/chain/skip.rb
index 38d10eef12fea6..91f6526921b6f8 100644
--- a/lib/gitlab/ci/pipeline/chain/skip.rb
+++ b/lib/gitlab/ci/pipeline/chain/skip.rb
@@ -29,7 +29,7 @@ def break?
           private
 
           def skipped?
-            !@command.ignore_skip_ci && (commit_message_skips_ci? || push_option_skips_ci?)
+            !@command.ignore_skip_ci && (commit_message_skips_ci? || !!@command.push_options&.skips_ci?)
           end
 
           def commit_message_skips_ci?
@@ -39,11 +39,6 @@ def commit_message_skips_ci?
               !!(@pipeline.git_commit_message =~ SKIP_PATTERN)
             end
           end
-
-          def push_option_skips_ci?
-            @command.push_options.present? &&
-              @command.push_options.deep_symbolize_keys.dig(:ci, :skip).present?
-          end
         end
       end
     end
diff --git a/spec/lib/ci/pipeline_creation/push_options_spec.rb b/spec/lib/ci/pipeline_creation/push_options_spec.rb
new file mode 100644
index 00000000000000..b131531a392020
--- /dev/null
+++ b/spec/lib/ci/pipeline_creation/push_options_spec.rb
@@ -0,0 +1,118 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'oj'
+
+RSpec.describe Ci::PipelineCreation::PushOptions, feature_category: :pipeline_composition do
+  let(:ci_push_options) { {} }
+
+  subject(:push_options) { described_class.new(ci_push_options) }
+
+  describe '#skips_ci?' do
+    context 'when there is no ci skip push option' do
+      it 'returns false' do
+        expect(push_options.skips_ci?).to be_falsey
+      end
+    end
+
+    context 'when there is a ci skip push option' do
+      let(:ci_push_options) { { ci: { skip: true } } }
+
+      it 'returns true' do
+        expect(push_options.skips_ci?).to be_truthy
+      end
+    end
+  end
+
+  describe '#variables' do
+    context 'when push options contain variables' do
+      let(:ci_push_options) do
+        {
+          ci: {
+            variable: {
+              "FOO=123": 1,
+              "BAR=456": 1,
+              "MNO=890=ABC": 1
+            }
+          }
+        }
+      end
+
+      it 'returns the extracted key value variable pairs from the push options' do
+        extracted_variables = [
+          { "key" => "FOO", "variable_type" => "env_var", "secret_value" => "123" },
+          { "key" => "BAR", "variable_type" => "env_var", "secret_value" => "456" },
+          { "key" => "MNO", "variable_type" => "env_var", "secret_value" => "890=ABC" }
+        ]
+
+        expect(push_options.variables).to eq(extracted_variables)
+      end
+    end
+
+    context 'when there are no variables in the push options' do
+      it 'returns an empty array' do
+        expect(push_options.variables).to eq([])
+      end
+    end
+
+    context 'when there are variables with a missing `key` or `value`' do
+      let(:ci_push_options) do
+        {
+          ci: {
+            variable: {
+              "=123": 1,
+              ABC: 1,
+              "ABC=": 1
+            }
+          }
+        }
+      end
+
+      it 'returns an empty string for the only valid format `KEY=`' do
+        variable = ["key" => "ABC", "secret_value" => "", "variable_type" => "env_var"]
+        expect(push_options.variables).to match_array(variable)
+      end
+    end
+  end
+
+  describe '#inputs' do
+    context 'when there are inputs in the push options' do
+      let(:ci_push_options) do
+        {
+          ci: {
+            input: {
+              'security_scan=false': 1,
+              'stage=test': 1,
+              'level=20': 1,
+              'environments=["staging", "production"]': 1,
+              'rules=[{"if": "$CI_MERGE_REQUEST_ID"}, {"if": "$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH"}]': 1
+            }
+          }
+        }
+      end
+
+      it 'returns the extracted key value input pairs from the push options' do
+        extracted_inputs =
+          {
+            security_scan: false,
+            stage: 'test',
+            level: 20,
+            environments: %w[
+              staging production
+            ],
+            rules: [
+              { if: "$CI_MERGE_REQUEST_ID" },
+              { if: "$CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH" }
+            ]
+          }
+        expect(push_options.inputs).to eq(extracted_inputs)
+      end
+    end
+
+    context 'when there are no inputs in the push options' do
+      it 'returns an empty hash' do
+        expect(push_options.inputs).to eq({})
+      end
+    end
+  end
+end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index a7de69d80bff48..8aa7ccf89c42e7 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -678,14 +678,10 @@ def previous_commit_sha_from_ref(ref)
     end
 
     context 'when push options contain ci.skip' do
-      let(:push_options) do
-        { 'ci' => { 'skip' => true } }
-      end
-
-      it 'creates a pipline in the skipped state' do
+      it 'creates a pipeline in the skipped state' do
+        push_options = Ci::PipelineCreation::PushOptions.new({ ci: { skip: true } })
         pipeline = execute_service(push_options: push_options).payload
 
-        # TODO: DRY these up with "skips builds creation if the commit message"
         expect(pipeline).to be_persisted
         expect(pipeline.builds.any?).to be false
         expect(pipeline.status).to eq("skipped")
diff --git a/spec/services/git/base_hooks_service_spec.rb b/spec/services/git/base_hooks_service_spec.rb
index a460319be4969a..33eaaddd7f6610 100644
--- a/spec/services/git/base_hooks_service_spec.rb
+++ b/spec/services/git/base_hooks_service_spec.rb
@@ -152,16 +152,16 @@ def commits_count
     end
   end
 
-  describe 'Pipeline options' do
-    context 'when pipeline options contain inputs' do
+  describe 'Pipeline push options' do
+    context 'when push options contain inputs' do
       let(:pipeline_params) do
         {
           after: newrev,
           before: oldrev,
           checkout_sha: checkout_sha,
-          push_options: push_options,
+          push_options: an_instance_of(Ci::PipelineCreation::PushOptions),
           ref: ref,
-          variables_attributes: variables_attributes
+          variables_attributes: []
         }
       end
 
@@ -182,7 +182,6 @@ def commits_count
       let(:pipeline_service) { double(execute: service_response) }
       let(:service_response) { double(error?: false, payload: pipeline, message: 'message') }
       let(:pipeline) { double(persisted?: true) }
-      let(:variables_attributes) { [] }
 
       let(:inputs) do
         {
@@ -204,6 +203,8 @@ def commits_count
       end
 
       it 'calls the create pipeline service' do
+        expect(Ci::PipelineCreation::PushOptions).to receive(:new).with(push_options).and_call_original
+
         expect(Ci::CreatePipelineService)
           .to receive(:new)
           .with(project, user, pipeline_params)
@@ -223,7 +224,7 @@ def commits_count
         after: newrev,
         before: oldrev,
         checkout_sha: checkout_sha,
-        push_options: push_options, # defined in each context
+        push_options: an_instance_of(Ci::PipelineCreation::PushOptions), # defined in each context
         ref: ref,
         variables_attributes: variables_attributes # defined in each context
       }
@@ -331,7 +332,7 @@ def commits_count
         after: newrev,
         before: oldrev,
         checkout_sha: checkout_sha,
-        push_options: push_options,
+        push_options: an_instance_of(Ci::PipelineCreation::PushOptions),
         ref: ref,
         variables_attributes: variables_attributes
       }
diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb
index ff2c535da0dcbb..a47d6c91ddcde2 100644
--- a/spec/services/git/branch_push_service_spec.rb
+++ b/spec/services/git/branch_push_service_spec.rb
@@ -87,7 +87,7 @@
           ref: ref,
           checkout_sha: SeedRepo::Commit::ID,
           variables_attributes: [],
-          push_options: {}
+          push_options: an_instance_of(::Ci::PipelineCreation::PushOptions)
         }
       ).and_call_original
 
-- 
GitLab


From d4f46eb73a280bd2b6ed661812ec14766d045d92 Mon Sep 17 00:00:00 2001
From: Laura Montemayor <lmontemayor@gitlab.com>
Date: Tue, 8 Apr 2025 14:05:35 +0200
Subject: [PATCH 2/2] Add fabricate to class and replace news

---
 app/services/ci/create_pipeline_service.rb    |  2 +-
 app/services/git/base_hooks_service.rb        |  4 +-
 .../merge_requests/create_pipeline_service.rb |  2 +-
 .../merge_requests/create_pipeline_service.rb |  2 +-
 lib/ci/pipeline_creation/push_options.rb      | 12 ++++++
 .../ci/pipeline_creation/push_options_spec.rb | 43 +++++++++++++++++++
 .../ci/create_pipeline_service_spec.rb        |  5 ++-
 7 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 07873ac27fca4b..a22c45fcb42cc6 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -93,7 +93,7 @@ def execute(
         variables_attributes: params[:variables_attributes],
         project: project,
         current_user: current_user,
-        push_options: params[:push_options] || ::Ci::PipelineCreation::PushOptions.new({}),
+        push_options: ::Ci::PipelineCreation::PushOptions.fabricate(params[:push_options]),
         chat_data: params[:chat_data],
         bridge: bridge,
         logger: @logger,
diff --git a/app/services/git/base_hooks_service.rb b/app/services/git/base_hooks_service.rb
index 957cc4d0ff34d7..64343d9c6f7036 100644
--- a/app/services/git/base_hooks_service.rb
+++ b/app/services/git/base_hooks_service.rb
@@ -108,7 +108,7 @@ def pipeline_params
 
     def ci_push_options
       strong_memoize(:ci_push_options) do
-        Ci::PipelineCreation::PushOptions.new(push_options)
+        Ci::PipelineCreation::PushOptions.fabricate(push_options)
       end
     end
 
@@ -120,7 +120,7 @@ def integration_push_options
 
     def push_options
       strong_memoize(:push_options) do
-        params[:push_options] || {}
+        params[:push_options]&.deep_symbolize_keys
       end
     end
 
diff --git a/app/services/merge_requests/create_pipeline_service.rb b/app/services/merge_requests/create_pipeline_service.rb
index 6f38ea0f981bb1..07390b4a6685aa 100644
--- a/app/services/merge_requests/create_pipeline_service.rb
+++ b/app/services/merge_requests/create_pipeline_service.rb
@@ -27,7 +27,7 @@ def create_detached_merge_request_pipeline(merge_request)
       Ci::CreatePipelineService.new(project,
         current_user,
         ref: ref,
-        push_options: ::Ci::PipelineCreation::PushOptions.new(params[:push_options]),
+        push_options: params[:push_options],
         pipeline_creation_request: params[:pipeline_creation_request]
       ).execute(:merge_request_event, merge_request: merge_request)
     end
diff --git a/ee/app/services/ee/merge_requests/create_pipeline_service.rb b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
index 3b2b3568123eeb..06c7905dfb8290 100644
--- a/ee/app/services/ee/merge_requests/create_pipeline_service.rb
+++ b/ee/app/services/ee/merge_requests/create_pipeline_service.rb
@@ -29,7 +29,7 @@ def create_merged_result_pipeline_for(merge_request)
               target_sha: ref_payload[:target_id],
               source_sha: ref_payload[:source_id],
               pipeline_creation_request: params[:pipeline_creation_request],
-              push_options: ::Ci::PipelineCreation::PushOptions.new(params[:push_options]))
+              push_options: params[:push_options])
             .execute(:merge_request_event, merge_request: merge_request)
         else
           cannot_create_pipeline_error
diff --git a/lib/ci/pipeline_creation/push_options.rb b/lib/ci/pipeline_creation/push_options.rb
index 08f9bb52b6b5de..68db659ae81ffe 100644
--- a/lib/ci/pipeline_creation/push_options.rb
+++ b/lib/ci/pipeline_creation/push_options.rb
@@ -3,6 +3,18 @@
 module Ci
   module PipelineCreation
     class PushOptions
+      def self.fabricate(push_options)
+        if push_options.is_a?(self)
+          push_options
+        elsif push_options.is_a?(Hash)
+          new(push_options)
+        elsif push_options.blank?
+          new({})
+        else
+          raise ArgumentError, 'Unknown type of push_option'
+        end
+      end
+
       def initialize(push_options)
         @push_options = push_options&.deep_symbolize_keys || {}
       end
diff --git a/spec/lib/ci/pipeline_creation/push_options_spec.rb b/spec/lib/ci/pipeline_creation/push_options_spec.rb
index b131531a392020..d7097866d07672 100644
--- a/spec/lib/ci/pipeline_creation/push_options_spec.rb
+++ b/spec/lib/ci/pipeline_creation/push_options_spec.rb
@@ -115,4 +115,47 @@
       end
     end
   end
+
+  describe '.fabricate' do
+    context 'when push options are a Hash' do
+      it 'creates a new instance of the PushOptions class' do
+        push_options = described_class.fabricate({ ci: { skip: true } })
+
+        expect(push_options.skips_ci?).to be_truthy
+        expect(push_options).to be_a described_class
+      end
+    end
+
+    context 'when push options are blank' do
+      it 'initalizes an empty instance of the PushOptions class' do
+        push_options = described_class.fabricate(nil)
+
+        expect(push_options).to be_a described_class
+      end
+    end
+
+    context 'when push options are already an instance of PushOption' do
+      it 'returns that instance' do
+        inputs =
+          {
+            ci: {
+              input: {
+                'security_scan=false': 1
+              }
+            }
+          }
+
+        existing_push_option = described_class.new(inputs)
+        push_options = described_class.fabricate(existing_push_option)
+
+        expect(push_options).to be(existing_push_option)
+      end
+    end
+
+    context 'when push options are none of these' do
+      it 'raises an Argument Error' do
+        expect { described_class.fabricate(1) }.to raise_error(ArgumentError, 'Unknown type of push_option')
+      end
+    end
+  end
 end
diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb
index 8aa7ccf89c42e7..178395cfdfb3b0 100644
--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -678,8 +678,11 @@ def previous_commit_sha_from_ref(ref)
     end
 
     context 'when push options contain ci.skip' do
+      let(:push_options) do
+        { 'ci' => { 'skip' => true } }
+      end
+
       it 'creates a pipeline in the skipped state' do
-        push_options = Ci::PipelineCreation::PushOptions.new({ ci: { skip: true } })
         pipeline = execute_service(push_options: push_options).payload
 
         expect(pipeline).to be_persisted
-- 
GitLab