From 84bbb4255678dea4dde1f753ac26468244126ebf Mon Sep 17 00:00:00 2001 From: Steven Pritchard Date: Thu, 30 Jul 2026 17:07:51 +0000 Subject: [PATCH 1/2] Add merge_gemfile task and pipeline stage (#50 slice 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second slice of the Renovate-resilient template work: with the Gemfile now bootstrap-mode (laid down only when missing), this is how baseline Gemfile changes reach existing repos — an in-place merge that never touches what Renovate manages. The merge_gemfile task: - Adds template gems missing from the target into their matching `group ... do` block (with attached comment lines, e.g. renovate manager hints); missing groups are copied verbatim at EOF - Brings along group-local variable assignments that inserted lines reference (transitively — `gem 'puppet', puppet_version` needs puppet_version; the pdk line needs major_puppet_version, which needs puppet_version), inserted at the top of the group in template order - Removes gems listed in remove_gems (plus an attached `# renovate:` comment line) - Never modifies existing gems' version constraints or any other existing content; writes the full template when the file is missing The plan stage resolves the template with the same per-module override chain as the profile (Gemfile. beats Gemfile), targets skeleton/Gemfile for pupmod_skeleton repos, runs only against pupmod-ish project types, and reads remove_gems from the session config (puppetsync.plans.sync.merge_gemfile.remove_gems). Verified with 11 new specs (driven by the real baseline template, including an eval check that merged output executes) and an e2e run: a fixture Gemfile with a custom simp-rake-helpers constraint and most of the baseline missing came out with all gems/groups/assignments added, the constraint byte-for-byte intact, remove_gems honored, and a second run reporting "1 unchanged". Refs #50 Co-Authored-By: Claude Fable 5 --- dist/puppetsync/plans/init.pp | 34 ++++ dist/puppetsync/tasks/merge_gemfile.json | 18 +++ dist/puppetsync/tasks/merge_gemfile.rb | 168 ++++++++++++++++++++ spec/tasks/merge_gemfile_spec.rb | 191 +++++++++++++++++++++++ 4 files changed, 411 insertions(+) create mode 100644 dist/puppetsync/tasks/merge_gemfile.json create mode 100644 dist/puppetsync/tasks/merge_gemfile.rb create mode 100644 spec/tasks/merge_gemfile_spec.rb diff --git a/dist/puppetsync/plans/init.pp b/dist/puppetsync/plans/init.pp index 28c1b3f..a7b7716 100644 --- a/dist/puppetsync/plans/init.pp +++ b/dist/puppetsync/plans/init.pp @@ -200,6 +200,40 @@ } } + $repos.puppetsync::pipeline_stage( + # -------------------------------------------------------------------------- + 'merge_gemfile', + # -------------------------------------------------------------------------- + $opts + ) |$ok_repos, $stage_name| { + # Top up existing (bootstrap-mode) Gemfiles in place: add gems the + # baseline requires, never touch existing version constraints + # (which Renovate manages). See simp/puppetsync#50. + $gemfile_repos = $ok_repos.filter |$repo| { + $repo.facts['project_type'] in ['pupmod', 'pupmod_skeleton'] + } + run_task_with('puppetsync::merge_gemfile', + $gemfile_repos, + '_catch_errors' => true, + ) |$repo| { + $gemfile_path = $repo.facts['project_type'] ? { + 'pupmod_skeleton' => "${repo.vars['repo_path']}/skeleton/Gemfile", + default => "${repo.vars['repo_path']}/Gemfile", + } + $target_module_name = $repo.facts.dig('module_metadata','name').lest || { + $repo.vars['mod_data']['repo_name'] + } + Hash({ + 'path' => $gemfile_path, + 'template' => file( + "profile/pupmod/Gemfile.${target_module_name}", + 'profile/pupmod/Gemfile', + ), + 'remove_gems' => $opts.dig('merge_gemfile', 'remove_gems').lest || {[]}, + }) + } + } + $repos.puppetsync::pipeline_stage( # -------------------------------------------------------------------------- 'drop_glci_config', diff --git a/dist/puppetsync/tasks/merge_gemfile.json b/dist/puppetsync/tasks/merge_gemfile.json new file mode 100644 index 0000000..c75fdc4 --- /dev/null +++ b/dist/puppetsync/tasks/merge_gemfile.json @@ -0,0 +1,18 @@ +{ + "description": "Merge the baseline Gemfile template into an existing Gemfile in place: add template gems that are missing (into their matching group), optionally remove gems, and never touch existing gems' version constraints (which Renovate manages). Writes the full template when the file doesn't exist.", + "input_method": "stdin", + "parameters": { + "path": { + "description": "Path to the target Gemfile", + "type": "String[1]" + }, + "template": { + "description": "Content of the baseline Gemfile template", + "type": "String[1]" + }, + "remove_gems": { + "description": "Names of gems to remove from the target (default: none)", + "type": "Optional[Array[String[1]]]" + } + } +} diff --git a/dist/puppetsync/tasks/merge_gemfile.rb b/dist/puppetsync/tasks/merge_gemfile.rb new file mode 100644 index 0000000..230b913 --- /dev/null +++ b/dist/puppetsync/tasks/merge_gemfile.rb @@ -0,0 +1,168 @@ +#!/opt/puppetlabs/bolt/bin/ruby +# +# Merge the baseline Gemfile template into an existing Gemfile IN PLACE: +# +# - Gems from the template that are missing from the target are added to +# the matching `group ... do` block (created at EOF when absent), +# together with any comment lines attached to them in the template +# - Gems in `remove_gems` are deleted (along with an attached +# `# renovate:` comment line) +# - Everything else — most importantly the version constraints of gems +# that already exist, which Renovate manages — is left untouched +# +# If the target file does not exist, the full template is written out +# (bootstrap behavior; see simp/puppetsync#50). + +require 'json' + +GEM_LINE_RE = /^\s*gem\s*\(?\s*['"](?[\w.-]+)['"]/ +GROUP_RE = /^\s*group\s+(?.+?)\s+do\s*$/ +# Statement block openers that require a matching `end` (inline modifiers +# like `gem 'x' if cond` deliberately don't match) +OPENER_RE = /(\bdo\s*(\|[^|]*\|)?\s*(#.*)?$)|(^\s*(if|unless|case|begin|module|class|def)\b)/ +END_RE = /^\s*end\b/ +# Local variable assignments (e.g. `puppet_version = ENV.fetch(...)`) that +# gem lines may reference; excludes == and =~ +ASSIGN_RE = /^\s*(?[a-z_]\w*)\s*=(?![=~])/ + +# Parse Gemfile text into: +# groups: { normalized_signature => { start:, end: } } (top-level groups only) +# gems: { gem_name => { index:, group: normalized_signature_or_nil } } +def parse_gemfile(lines) + groups = {} + gems = {} + stack = [] + + lines.each_with_index do |line, idx| + if (m = line.match(GEM_LINE_RE)) + enclosing_group = stack.reverse.find { |frame| frame[:group] } + gems[m[:name]] ||= { index: idx, group: enclosing_group&.dig(:sig) } + end + + if (m = line.match(GROUP_RE)) + sig = m[:sig].strip + stack.push({ group: true, sig: sig, start: idx }) + elsif line.match?(END_RE) + frame = stack.pop + groups[frame[:sig]] = { start: frame[:start], end: idx } if frame && frame[:group] && stack.empty? + elsif line.match?(OPENER_RE) + stack.push({ group: false }) + end + end + + { groups: groups, gems: gems } +end + +# Comment lines immediately above index `idx` (attached documentation, e.g. +# `# renovate:` manager hints) +def attached_comments(lines, idx) + first = idx + first -= 1 while first.positive? && lines[first - 1].match?(/^\s*#/) + (first...idx).map { |i| lines[i] } +end + +def merge_gemfile(path, template, remove_gems) + unless File.exist?(path) + File.write(path, template) + added = template.split("\n").filter_map { |l| l.match(GEM_LINE_RE)&.[](:name) } + return { 'changed' => true, 'created' => true, 'added' => added, 'removed' => [] } + end + + original = File.read(path) + lines = original.split("\n", -1) + template_lines = template.split("\n", -1) + template_parsed = parse_gemfile(template_lines) + + added = [] + removed = [] + + # --- Removals --------------------------------------------------------- + remove_gems.each do |name| + parsed = parse_gemfile(lines) + next unless parsed[:gems].key?(name) + + idx = parsed[:gems][name][:index] + first = idx + first -= 1 if first.positive? && lines[first - 1].match?(/^\s*#\s*renovate:/) + lines.slice!(first..idx) + removed << name + end + + # --- Additions -------------------------------------------------------- + template_parsed[:gems].each do |name, tmeta| + next if remove_gems.include?(name) + + parsed = parse_gemfile(lines) + next if parsed[:gems].key?(name) + + insert_lines = attached_comments(template_lines, tmeta[:index]) + [template_lines[tmeta[:index]]] + + if tmeta[:group].nil? + # Top-level gem: after the last top-level gem, else before the first + # group, else EOF + top_level = parsed[:gems].values.select { |g| g[:group].nil? }.map { |g| g[:index] } + insert_at = if top_level.any? + top_level.max + 1 + elsif parsed[:groups].any? + parsed[:groups].values.map { |g| g[:start] }.min + else + lines.length + end + elsif (target_group = parsed[:groups][tmeta[:group]]) + group_gems = parsed[:gems].values.select { |g| g[:group] == tmeta[:group] }.map { |g| g[:index] } + insert_at = group_gems.any? ? group_gems.max + 1 : target_group[:start] + 1 + else + # Group missing entirely: copy the template's whole block verbatim at + # EOF (so group-local variable assignments etc. come along) + tgroup = template_parsed[:groups][tmeta[:group]] + block = template_lines[tgroup[:start]..tgroup[:end]] + lines << '' unless lines.last.to_s.empty? + lines.concat(block + ['']) + added.concat(block.filter_map { |l| l.match(GEM_LINE_RE)&.[](:name) }) + next + end + + lines.insert(insert_at, *insert_lines) + added << name + + # Ensure any group-local variable assignments the inserted lines + # reference (e.g. `gem 'puppet', puppet_version`) exist in the target. + # References are transitive (an assignment may reference an earlier + # one); insert them at the top of the group in template order. + tgroup = template_parsed[:groups][tmeta[:group]] + group_body = template_lines[(tgroup[:start] + 1)...tgroup[:end]] + assignments = group_body.filter_map { |l| (v = l.match(ASSIGN_RE)&.[](:var)) && [v, l] }.to_h + + referenced_text = insert_lines.join("\n") + needed_vars = [] + loop do + new_vars = assignments.keys.reject { |v| needed_vars.include?(v) } + .select { |v| referenced_text.match?(/\b#{Regexp.escape(v)}\b/) } + break if new_vars.empty? + + needed_vars.concat(new_vars) + referenced_text += "\n#{new_vars.map { |v| assignments[v] }.join("\n")}" + end + + needed = group_body.select do |tline| + var = tline.match(ASSIGN_RE)&.[](:var) + var && needed_vars.include?(var) && lines.none? { |l| l.match(ASSIGN_RE)&.[](:var) == var } + end + lines.insert(target_group[:start] + 1, *needed) if needed.any? + end + + content = lines.join("\n") + changed = content != original + File.write(path, content) if changed + { 'changed' => changed, 'added' => added, 'removed' => removed } +end + +stdin = STDIN.read +params = JSON.parse(stdin) +warn stdin + +raise('No path given') unless params['path'] +raise('No template given') unless params['template'] +remove_gems = params.fetch('remove_gems', nil) || [] + +puts JSON.generate(merge_gemfile(params['path'], params['template'], remove_gems)) diff --git a/spec/tasks/merge_gemfile_spec.rb b/spec/tasks/merge_gemfile_spec.rb new file mode 100644 index 0000000..d96b265 --- /dev/null +++ b/spec/tasks/merge_gemfile_spec.rb @@ -0,0 +1,191 @@ +require 'spec_helper' + +describe 'task: merge_gemfile' do + let(:real_template) do + File.read(File.join(REPO_ROOT, 'modules', 'profile', 'files', 'pupmod', 'Gemfile')) + end + + around(:each) do |example| + Dir.mktmpdir do |dir| + @gemfile = File.join(dir, 'Gemfile') + example.run + end + end + + def run_merge(template:, remove_gems: nil) + params = { 'path' => @gemfile, 'template' => template } + params['remove_gems'] = remove_gems if remove_gems + run_task('merge_gemfile.rb', params) + end + + it 'writes the full template when the target does not exist' do + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + result = JSON.parse(stdout) + expect(result['changed']).to be true + expect(result['created']).to be true + expect(File.read(@gemfile)).to eq(real_template) + end + + context 'with a target derived from the baseline' do + before(:each) { File.write(@gemfile, real_template) } + + it 'is a no-op on an identical file' do + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)).to include('changed' => false, 'added' => [], 'removed' => []) + expect(File.read(@gemfile)).to eq(real_template) + end + + it 'never touches an existing gem version constraint' do + # Simulate Renovate having bumped a pinned constraint + munged = real_template.sub( + "gem 'simp-rake-helpers', ENV.fetch('SIMP_RAKE_HELPERS_VERSION', ['>= 5.21.0', '< 6'])", + "gem 'simp-rake-helpers', ENV.fetch('SIMP_RAKE_HELPERS_VERSION', ['>= 5.24.0', '< 7'])", + ) + raise 'munge failed' if munged == real_template + File.write(@gemfile, munged) + + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)['changed']).to be false + expect(File.read(@gemfile)).to include("['>= 5.24.0', '< 7']") + end + + it 'adds a template gem missing from its group, at the end of that group' do + File.write(@gemfile, real_template.sub(/^\s*gem 'rake'\n/, '')) + + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)).to include('changed' => true, 'added' => ['rake']) + content = File.read(@gemfile) + test_group = content[/^group :test do.*?^end/m] + expect(test_group).to include("gem 'rake'") + end + + it 'adds attached comment lines together with the gem' do + template = <<~GEMFILE + source 'https://rubygems.org' + + group :test do + # renovate: datasource=rubygems versioning=ruby + gem 'new-gem', ENV.fetch('NEW_GEM_VERSION', '~> 1.0') + gem 'rake' + end + GEMFILE + File.write(@gemfile, <<~GEMFILE) + source 'https://rubygems.org' + + group :test do + gem 'rake' + end + GEMFILE + + stdout, stderr, status = run_merge(template: template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)['added']).to eq(['new-gem']) + content = File.read(@gemfile) + expect(content.lines.map(&:rstrip)).to include( + ' # renovate: datasource=rubygems versioning=ruby', + " gem 'new-gem', ENV.fetch('NEW_GEM_VERSION', '~> 1.0')", + ) + end + + it 'creates a missing group block at EOF' do + no_system_tests = real_template.sub(/^group :system_tests do.*?^end\n/m, '') + raise 'munge failed' if no_system_tests == real_template + File.write(@gemfile, no_system_tests) + + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + result = JSON.parse(stdout) + expect(result['changed']).to be true + expect(result['added']).to include('beaker', 'simp-beaker-helpers') + content = File.read(@gemfile) + expect(content[/^group :system_tests do.*?^end/m]).to include("gem 'beaker'") + end + + it 'removes gems in remove_gems along with an attached renovate comment' do + with_extra = real_template.sub( + "group :test do\n", + "group :test do\n # renovate: datasource=rubygems versioning=ruby\n gem 'obsolete-lint-check', '~> 1.0'\n", + ) + File.write(@gemfile, with_extra) + + stdout, stderr, status = run_merge(template: real_template, remove_gems: ['obsolete-lint-check']) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)).to include('changed' => true, 'removed' => ['obsolete-lint-check']) + content = File.read(@gemfile) + expect(content).not_to include('obsolete-lint-check') + expect(content.scan(/# renovate:/).count).to eq(real_template.scan(/# renovate:/).count) + end + + it 'preserves custom gems the template knows nothing about' do + with_custom = real_template.sub("group :development do\n", "group :development do\n gem 'my-local-tool', path: '../tool'\n") + File.write(@gemfile, with_custom) + + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)['changed']).to be false + expect(File.read(@gemfile)).to include("gem 'my-local-tool', path: '../tool'") + end + + it 'brings referenced variable assignments along with added gems (transitively)' do + File.write(@gemfile, <<~GEMFILE) + source 'https://rubygems.org' + + group :test do + gem 'rake' + end + GEMFILE + + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)['added']).to include('puppet', 'pdk') + content = File.read(@gemfile) + test_group = content[/^group :test do.*?^end/m] + # `gem 'puppet', puppet_version` requires puppet_version; the pdk line + # requires major_puppet_version, which itself requires puppet_version + expect(test_group.index('puppet_version =')).to be < test_group.index('major_puppet_version =') + expect(test_group.index('major_puppet_version =')).to be < test_group.index("gem 'puppet'") + + # The merged result must actually evaluate (undefined locals raise) + evaluator = <<~RUBY + def source(*); end + def group(*); yield; end + def gem(*, **); end + eval(File.read(#{@gemfile.inspect})) + puts 'EVAL_OK' + RUBY + out, eval_status = Open3.capture2e(RbConfig.ruby, '-e', evaluator) + expect(eval_status).to be_success, out + expect(out).to include('EVAL_OK') + end + + it 'is idempotent' do + File.write(@gemfile, real_template.sub(/^\s*gem 'rake'\n/, '')) + run_merge(template: real_template) + first_pass = File.read(@gemfile) + + stdout, stderr, status = run_merge(template: real_template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)['changed']).to be false + expect(File.read(@gemfile)).to eq(first_pass) + end + end + + it 'fails when required params are missing' do + _stdout, _stderr, status = run_task('merge_gemfile.rb', 'path' => @gemfile) + expect(status).not_to be_success + end +end From 349c8cfac6778cfd9908430ba71c0a865a944bb4 Mon Sep 17 00:00:00 2001 From: Steven Pritchard Date: Fri, 31 Jul 2026 14:20:07 +0000 Subject: [PATCH 2/2] Address review feedback: drop debug output, guard top-level gem additions - Remove the `warn stdin` debug line (it echoed the full template to stderr on every invocation) - Guard the group-local assignment block for top-level gems: it dereferenced template_parsed[:groups][nil], so the first bare `gem 'foo'` added to the baseline template would have raised NoMethodError on every existing repo. Covered by a new spec (the one addition case the suite didn't exercise) Co-Authored-By: Claude Fable 5 --- dist/puppetsync/tasks/merge_gemfile.rb | 4 +++- spec/tasks/merge_gemfile_spec.rb | 27 ++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/dist/puppetsync/tasks/merge_gemfile.rb b/dist/puppetsync/tasks/merge_gemfile.rb index 230b913..c3182fc 100644 --- a/dist/puppetsync/tasks/merge_gemfile.rb +++ b/dist/puppetsync/tasks/merge_gemfile.rb @@ -125,6 +125,9 @@ def merge_gemfile(path, template, remove_gems) lines.insert(insert_at, *insert_lines) added << name + # Top-level gems have no group body to source assignments from + next if tmeta[:group].nil? + # Ensure any group-local variable assignments the inserted lines # reference (e.g. `gem 'puppet', puppet_version`) exist in the target. # References are transitive (an assignment may reference an earlier @@ -159,7 +162,6 @@ def merge_gemfile(path, template, remove_gems) stdin = STDIN.read params = JSON.parse(stdin) -warn stdin raise('No path given') unless params['path'] raise('No template given') unless params['template'] diff --git a/spec/tasks/merge_gemfile_spec.rb b/spec/tasks/merge_gemfile_spec.rb index d96b265..744dba9 100644 --- a/spec/tasks/merge_gemfile_spec.rb +++ b/spec/tasks/merge_gemfile_spec.rb @@ -171,6 +171,33 @@ def gem(*, **); end expect(out).to include('EVAL_OK') end + it 'adds a top-level (ungrouped) template gem without raising' do + template = <<~GEMFILE + source 'https://rubygems.org' + + gem 'rake' + + group :test do + gem 'rspec' + end + GEMFILE + File.write(@gemfile, <<~GEMFILE) + source 'https://rubygems.org' + + group :test do + gem 'rspec' + end + GEMFILE + + stdout, stderr, status = run_merge(template: template) + + expect(status).to be_success, stderr + expect(JSON.parse(stdout)).to include('changed' => true, 'added' => ['rake']) + content = File.read(@gemfile) + # Inserted at the top level, before the first group + expect(content.index("gem 'rake'")).to be < content.index('group :test') + end + it 'is idempotent' do File.write(@gemfile, real_template.sub(/^\s*gem 'rake'\n/, '')) run_merge(template: real_template)