From 8b2c0137f3e313c549f93ccc5d1a6b3a895a3fc5 Mon Sep 17 00:00:00 2001 From: Isaiah Frantz Date: Tue, 27 Jan 2026 12:31:27 -0800 Subject: [PATCH 1/2] Fix fact-override option to preserve JSON values with commas Replace option_globally_or_per_branch with explicit parser.on handlers to avoid automatic Array parsing that splits on commas, allowing JSON fact overrides containing commas to remain intact. --- .../cli/options/fact_override.rb | 23 ++++++++++++------- .../tests/cli/options/fact_override_spec.rb | 7 ++++++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/octocatalog-diff/cli/options/fact_override.rb b/lib/octocatalog-diff/cli/options/fact_override.rb index 6e8c5d0c..e06e6dee 100644 --- a/lib/octocatalog-diff/cli/options/fact_override.rb +++ b/lib/octocatalog-diff/cli/options/fact_override.rb @@ -9,13 +9,20 @@ def parse(parser, options) # Set 'fact_override_in' because more processing is needed, once the command line options # have been parsed, to make this into the final form 'fact_override'. - OctocatalogDiff::Cli::Options.option_globally_or_per_branch( - parser: parser, - options: options, - cli_name: 'fact-override', - option_name: 'fact_override_in', - desc: 'Override fact', - datatype: [] - ) + # Avoid Array parsing here so JSON values with commas stay intact. + parser.on('--fact-override STRING', 'Override fact globally') do |x| + options[:to_fact_override_in] ||= [] + options[:from_fact_override_in] ||= [] + options[:to_fact_override_in] << x + options[:from_fact_override_in] << x + end + parser.on('--to-fact-override STRING', 'Override fact for the to branch') do |x| + options[:to_fact_override_in] ||= [] + options[:to_fact_override_in] << x + end + parser.on('--from-fact-override STRING', 'Override fact for the from branch') do |x| + options[:from_fact_override_in] ||= [] + options[:from_fact_override_in] << x + end end end diff --git a/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb b/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb index e16fccdf..f5fb53d6 100644 --- a/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb +++ b/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb @@ -11,5 +11,12 @@ result = run_optparse(args) expect(result[:to_fact_override_in]).to eq(['foo=bar', 'baz=buzz']) end + + it 'should not split JSON overrides containing commas' do + args = ['--fact-override', 'json_list=(json)["a","b","c"]'] + result = run_optparse(args) + expect(result[:to_fact_override_in]).to eq(['json_list=(json)["a","b","c"]']) + expect(result[:from_fact_override_in]).to eq(['json_list=(json)["a","b","c"]']) + end end end From be1e72be0dc592f91cc17bcaf1382faa8ac74182 Mon Sep 17 00:00:00 2001 From: Isaiah Frantz Date: Tue, 27 Jan 2026 13:11:47 -0800 Subject: [PATCH 2/2] Support comma-separated lists for fact-override and enc-override options Add split_override_list method to parse comma-separated override values while preserving commas inside JSON arrays and objects. Update fact-override and enc-override options to accept comma-separated lists using explicit parser.on handlers. Add tests for comma-separated parsing and JSON value preservation. Update documentation to describe comma-separated list support. --- doc/advanced-override-facts.md | 2 + doc/optionsref.md | 2 +- lib/octocatalog-diff/cli/options.rb | 65 +++++++++++++++++++ .../cli/options/enc_override.rb | 26 +++++--- .../cli/options/fact_override.rb | 16 +++-- .../tests/cli/options/enc_override_spec.rb | 14 ++++ .../tests/cli/options/fact_override_spec.rb | 14 ++++ 7 files changed, 121 insertions(+), 18 deletions(-) diff --git a/doc/advanced-override-facts.md b/doc/advanced-override-facts.md index d9eda607..9e372905 100644 --- a/doc/advanced-override-facts.md +++ b/doc/advanced-override-facts.md @@ -18,6 +18,8 @@ To override a fact in the "from" catalog: You may use as many of these arguments as you wish to adjust as many facts as you wish. +These options support specifying multiple overrides as a comma-separated list. Commas inside JSON values (for example, arrays) will not be treated as separators. You may also repeat the option. + ## Examples Simulate a change to its IP address in the "to" branch: diff --git a/doc/optionsref.md b/doc/optionsref.md index e73f6a5d..b7670e53 100644 --- a/doc/optionsref.md +++ b/doc/optionsref.md @@ -68,9 +68,9 @@ Usage: octocatalog-diff [command line options] References to validate --[no-]compare-file-text[=force] Compare text, not source location, of file resources + --[no-]storeconfigs Enable integration with puppetdb for collected resources --storeconfigs-backend TERMINUS Set the terminus used for storeconfigs - --[no-]storeconfigs Enable integration with puppetdb for collected resources --retry-failed-catalog N Retry building a failed catalog N times --no-enc Disable ENC --enc PATH Path to ENC script, relative to checkout directory or absolute diff --git a/lib/octocatalog-diff/cli/options.rb b/lib/octocatalog-diff/cli/options.rb index e2956420..9529a81b 100644 --- a/lib/octocatalog-diff/cli/options.rb +++ b/lib/octocatalog-diff/cli/options.rb @@ -178,6 +178,71 @@ def self.option_globally_or_per_branch_array(opts = {}) end end + def self.split_override_list(input) + s = input.to_s + parts = [] + buf = String.new + square_depth = 0 + curly_depth = 0 + in_quote = nil + escaped = false + + s.each_char do |ch| + if escaped + buf << ch + escaped = false + next + end + + if ch == '\\' + buf << ch + escaped = true + next + end + + if in_quote + in_quote = nil if ch == in_quote + buf << ch + next + end + + if ch == '"' || ch == "'" + in_quote = ch + buf << ch + next + end + + case ch + when '[' + square_depth += 1 + buf << ch + when ']' + square_depth -= 1 if square_depth.positive? + buf << ch + when '{' + curly_depth += 1 + buf << ch + when '}' + curly_depth -= 1 if curly_depth.positive? + buf << ch + when ',' + if square_depth.zero? && curly_depth.zero? + part = buf.strip + parts << part unless part.empty? + buf = String.new + else + buf << ch + end + else + buf << ch + end + end + + part = buf.strip + parts << part unless part.empty? + parts + end + # See description of `option_globally_or_per_branch`. This implements the logic for a boolean value. # @param :parser [OptionParser object] The OptionParser argument # @param :options [Hash] Options hash being constructed; this is modified in this method. diff --git a/lib/octocatalog-diff/cli/options/enc_override.rb b/lib/octocatalog-diff/cli/options/enc_override.rb index fdda908d..5fadb6c5 100644 --- a/lib/octocatalog-diff/cli/options/enc_override.rb +++ b/lib/octocatalog-diff/cli/options/enc_override.rb @@ -7,15 +7,21 @@ has_weight 322 def parse(parser, options) - # Set 'enc_override_in' because more processing is needed, once the command line options - # have been parsed, to make this into the final form 'enc_override'. - OctocatalogDiff::Cli::Options.option_globally_or_per_branch( - parser: parser, - options: options, - cli_name: 'enc-override', - option_name: 'enc_override_in', - desc: 'Override parameter from ENC', - datatype: [] - ) + parser.on('--enc-override STRING1[,STRING2[,...]]', 'Override parameter from ENC globally') do |x| + options[:to_enc_override_in] ||= [] + options[:from_enc_override_in] ||= [] + OctocatalogDiff::Cli::Options.split_override_list(x).each do |item| + options[:to_enc_override_in] << item + options[:from_enc_override_in] << item + end + end + parser.on('--to-enc-override STRING1[,STRING2[,...]]', 'Override parameter from ENC for the to branch') do |x| + options[:to_enc_override_in] ||= [] + OctocatalogDiff::Cli::Options.split_override_list(x).each { |item| options[:to_enc_override_in] << item } + end + parser.on('--from-enc-override STRING1[,STRING2[,...]]', 'Override parameter from ENC for the from branch') do |x| + options[:from_enc_override_in] ||= [] + OctocatalogDiff::Cli::Options.split_override_list(x).each { |item| options[:from_enc_override_in] << item } + end end end diff --git a/lib/octocatalog-diff/cli/options/fact_override.rb b/lib/octocatalog-diff/cli/options/fact_override.rb index e06e6dee..df0db243 100644 --- a/lib/octocatalog-diff/cli/options/fact_override.rb +++ b/lib/octocatalog-diff/cli/options/fact_override.rb @@ -10,19 +10,21 @@ def parse(parser, options) # Set 'fact_override_in' because more processing is needed, once the command line options # have been parsed, to make this into the final form 'fact_override'. # Avoid Array parsing here so JSON values with commas stay intact. - parser.on('--fact-override STRING', 'Override fact globally') do |x| + parser.on('--fact-override STRING1[,STRING2[,...]]', 'Override fact globally') do |x| options[:to_fact_override_in] ||= [] options[:from_fact_override_in] ||= [] - options[:to_fact_override_in] << x - options[:from_fact_override_in] << x + OctocatalogDiff::Cli::Options.split_override_list(x).each do |item| + options[:to_fact_override_in] << item + options[:from_fact_override_in] << item + end end - parser.on('--to-fact-override STRING', 'Override fact for the to branch') do |x| + parser.on('--to-fact-override STRING1[,STRING2[,...]]', 'Override fact for the to branch') do |x| options[:to_fact_override_in] ||= [] - options[:to_fact_override_in] << x + OctocatalogDiff::Cli::Options.split_override_list(x).each { |item| options[:to_fact_override_in] << item } end - parser.on('--from-fact-override STRING', 'Override fact for the from branch') do |x| + parser.on('--from-fact-override STRING1[,STRING2[,...]]', 'Override fact for the from branch') do |x| options[:from_fact_override_in] ||= [] - options[:from_fact_override_in] << x + OctocatalogDiff::Cli::Options.split_override_list(x).each { |item| options[:from_fact_override_in] << item } end end end diff --git a/spec/octocatalog-diff/tests/cli/options/enc_override_spec.rb b/spec/octocatalog-diff/tests/cli/options/enc_override_spec.rb index 185bfbfa..3f3a3e79 100644 --- a/spec/octocatalog-diff/tests/cli/options/enc_override_spec.rb +++ b/spec/octocatalog-diff/tests/cli/options/enc_override_spec.rb @@ -11,5 +11,19 @@ result = run_optparse(args) expect(result[:to_enc_override_in]).to eq(['foo=bar', 'baz=buzz']) end + + it 'should accept a comma-separated list of overrides' do + args = ['--enc-override', 'foo=bar,baz=buzz'] + result = run_optparse(args) + expect(result[:to_enc_override_in]).to eq(['foo=bar', 'baz=buzz']) + expect(result[:from_enc_override_in]).to eq(['foo=bar', 'baz=buzz']) + end + + it 'should split comma-separated overrides but not split commas inside JSON' do + args = ['--enc-override', 'foo=bar,json_list=(json)["a","b"],baz=buzz'] + result = run_optparse(args) + expect(result[:to_enc_override_in]).to eq(['foo=bar', 'json_list=(json)["a","b"]', 'baz=buzz']) + expect(result[:from_enc_override_in]).to eq(['foo=bar', 'json_list=(json)["a","b"]', 'baz=buzz']) + end end end diff --git a/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb b/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb index f5fb53d6..8e5627d3 100644 --- a/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb +++ b/spec/octocatalog-diff/tests/cli/options/fact_override_spec.rb @@ -18,5 +18,19 @@ expect(result[:to_fact_override_in]).to eq(['json_list=(json)["a","b","c"]']) expect(result[:from_fact_override_in]).to eq(['json_list=(json)["a","b","c"]']) end + + it 'should accept a comma-separated list of overrides' do + args = ['--fact-override', 'foo=bar,baz=buzz'] + result = run_optparse(args) + expect(result[:to_fact_override_in]).to eq(['foo=bar', 'baz=buzz']) + expect(result[:from_fact_override_in]).to eq(['foo=bar', 'baz=buzz']) + end + + it 'should split comma-separated overrides but not split commas inside JSON' do + args = ['--fact-override', 'foo=bar,json_list=(json)["a","b"],baz=buzz'] + result = run_optparse(args) + expect(result[:to_fact_override_in]).to eq(['foo=bar', 'json_list=(json)["a","b"]', 'baz=buzz']) + expect(result[:from_fact_override_in]).to eq(['foo=bar', 'json_list=(json)["a","b"]', 'baz=buzz']) + end end end