From 8a06566100d8961b2916bfc19037f95d2dcb2f0b Mon Sep 17 00:00:00 2001 From: Nick Bertrand Date: Wed, 30 Mar 2022 13:20:09 -0500 Subject: [PATCH 1/2] (PA-5187) Add target support Prior to this commit, the target parameter of a yumrepo resource was ignored. This commit enables the target parameter to set the filepath for a yum repository to an arbitrary filepath or to an existing repository file. Co-authored-by: Michael Hashizume --- lib/puppet/provider/yumrepo/inifile.rb | 127 +++++++++-- lib/puppet/type/yumrepo.rb | 4 +- spec/acceptance/tests/yumrepo_spec.rb | 34 ++- spec/unit/provider/yumrepo/inifile_spec.rb | 244 ++++++++++++++++++--- 4 files changed, 342 insertions(+), 67 deletions(-) diff --git a/lib/puppet/provider/yumrepo/inifile.rb b/lib/puppet/provider/yumrepo/inifile.rb index 93e9bfe..7769d8c 100644 --- a/lib/puppet/provider/yumrepo/inifile.rb +++ b/lib/puppet/provider/yumrepo/inifile.rb @@ -1,5 +1,59 @@ require 'puppet/util/inifile' +module Puppet::Provider::Yumrepo + module IniConfig + class Section < Puppet::Util::IniConfig::Section; end + + # Examines a file on disk and identifies whether a section exists. + class PhysicalFile < Puppet::Util::IniConfig::PhysicalFile + def store + unlinked = false + if @destroy_empty && (sections.empty? || sections.all?(&:destroy?)) + ::File.unlink(@file) + unlinked = true + elsif sections.any?(&:dirty?) + text = self.format + @filetype.write(text) + end + sections.each(&:mark_clean) + unlinked + end + + private + + def section_exists?(name) + section = @file_collection.get_section(name) if @file_collection + if get_section(name) + true + elsif section && !section.destroy? + true + else + false + end + end + end + + # Creates a collection of new files on disk. + class FileCollection < Puppet::Util::IniConfig::FileCollection + def store + @files.delete_if do |_, file| + file.store + end + end + + private + + # Create a new physical file and set required attributes on that file. + def new_physical_file(file) + @files[file] = PhysicalFile.new(file, destroy_empty: true) + @files[file].file_collection = self + @files[file] + end + end + File = FileCollection + end +end + Puppet::Type.type(:yumrepo).provide(:inifile) do desc <<-EOD Manage yum repo configurations by parsing yum INI configuration files. @@ -35,7 +89,7 @@ def self.instances # Ignore the 'main' section in yum.conf since it's not a repository. next if section.name == 'main' - attributes_hash = { name: section.name, ensure: :present, provider: :yumrepo } + attributes_hash = { name: section.name, target: section.file, ensure: :present, provider: :yumrepo } section.entries.each do |key, value| key = key.to_sym @@ -104,7 +158,7 @@ def self.clear def self.find_conf_value(value, conf = '/etc/yum.conf') return unless Puppet::FileSystem.exist?(conf) - file = Puppet::Util::IniConfig::PhysicalFile.new(conf) + file = Puppet::Provider::Yumrepo::IniConfig::PhysicalFile.new(conf) file.read main = file.get_section('main') main ? main[value] : nil @@ -129,11 +183,11 @@ def self.repofiles # Build a virtual inifile by reading in numerous .repo files into a single # virtual file to ease manipulation. # @api private - # @return [Puppet::Util::IniConfig::File] The virtual inifile representing + # @return [Puppet::Provider::Yumrepo::IniConfig::File] The virtual inifile representing # multiple real files. def self.virtual_inifile unless @virtual - @virtual = Puppet::Util::IniConfig::File.new + @virtual = Puppet::Provider::Yumrepo::IniConfig::File.new repofiles.each do |file| @virtual.read(file) if Puppet::FileSystem.file?(file) end @@ -159,13 +213,26 @@ def self.valid_property?(key) # /etc/yum.conf is used. # # @param name [String] Section name to lookup in the virtual inifile. - # @return [Puppet::Util::IniConfig] The IniConfig section - def self.section(name) - result = virtual_inifile[name] + # @return [Puppet::Provider::Yumrepo::IniConfig::Section] The IniConfig section + def self.section(name, target) + path = repo_path(name, target) + result = nil + old_section = nil + virtual_inifile.each_section do |section| + if section.name.eql?(name) + if target.nil? || target.eql?(:absent) || section.file.eql?(path) + result = section + else + old_section = section + section.destroy = true + end + end + end # Create a new section if not found. unless result - path = repo_path(name) result = virtual_inifile.add_section(name, path) + # Copy section from old target if found. + old_section&.entries&.each { |entry| result.add_line(entry) } end result end @@ -188,15 +255,30 @@ def self.store(resource) end end - def self.repo_path(name) + def self.repo_path(name, target) dirs = reposdir path = if dirs.empty? # If no repo directories are present, default to using yum.conf. + unless target.nil? || target.eql?(:absent) + Puppet.debug("Using /etc/yum.conf instead of target #{target}") + end '/etc/yum.conf' - else - # The ordering of reposdir is [defaults, custom], and we want to use - # the custom directory if present. + # The ordering of reposdir is [defaults, custom], and we want to use + # the custom directory if present.else + elsif target.nil? || target.eql?(:absent) File.join(dirs.last, "#{name}.repo") + else + parent = Puppet::FileSystem.dir_string(target) + basename = Puppet::FileSystem.basename_string(target) + suffix = target.end_with?('.repo') ? '' : '.repo' + if parent == basename + File.join(dirs.last, "#{target}#{suffix}") + elsif dirs.include?(parent) + "#{target}#{suffix}" + else + Puppet.debug("Parent directory of #{target} not a valid yum directory.") + File.join(dirs.last, "#{basename}#{suffix}") + end end path end @@ -208,11 +290,12 @@ def self.repo_path(name) # @return [void] def create @property_hash[:ensure] = :present + self.target = self.class.repo_path(name, @resource[:target]) unless @resource[:target].eql?(:absent) # Check to see if the file that would be created in the # default location for the yumrepo already exists on disk. # If it does, read it in to the virtual inifile - path = self.class.repo_path(name) + path = self.class.repo_path(name, target) self.class.virtual_inifile.read(path) if Puppet::FileSystem.file?(path) # We fetch a list of properties from the type, then iterate @@ -232,7 +315,7 @@ def create # @api public # @return [Boolean] def exists? - @property_hash[:ensure] == :present + @property_hash[:ensure] == :present and (@resource[:target].eql?(:absent) or @property_hash[:target] == self.class.repo_path(name, @resource[:target])) end # Mark the given repository section for destruction. @@ -287,6 +370,18 @@ def descr=(value) @property_hash[:descr] = value end + def target + unless @property_hash.key?(:target) + @property_hash[:target] = self.class.repo_path(name, nil) + end + value = @property_hash[:target] + value.nil? ? :absent : value + end + + def target=(value) + @property_hash[:target] = value + end + private def get_property(property) @@ -304,10 +399,10 @@ def set_property(property, value) end def section(name) - self.class.section(name) + self.class.section(name, target) end def current_section - self.class.section(name) + self.class.section(name, target) end end diff --git a/lib/puppet/type/yumrepo.rb b/lib/puppet/type/yumrepo.rb index 712d89f..f6d666a 100644 --- a/lib/puppet/type/yumrepo.rb +++ b/lib/puppet/type/yumrepo.rb @@ -43,9 +43,7 @@ end newparam(:target) do - desc 'The target parameter will be enabled in a future release and should not be used.' - - defaultto :absent + desc 'The filepath of the local repository file, can be either relative or absolute. If a valid filepath is not specified, the target is created as a new section in `yum.conf(5)`' end newproperty(:descr) do diff --git a/spec/acceptance/tests/yumrepo_spec.rb b/spec/acceptance/tests/yumrepo_spec.rb index 92bf35c..a84108e 100644 --- a/spec/acceptance/tests/yumrepo_spec.rb +++ b/spec/acceptance/tests/yumrepo_spec.rb @@ -1,11 +1,8 @@ require 'spec_helper_acceptance' -# The target parameter doesn't work (see PUP-2782) so this creates two -# files instead of two repo entries in the one target file -products_repo = '/etc/yum.repos.d/puppetrepo-products.repo' -deps_repo = '/etc/yum.repos.d/puppetrepo-deps.repo' +puppet_repo = '/etc/yum.repos.d/puppetlabs.repo' manifest = < 'puppetrepo-products', descr => 'Puppet Labs Products El 7 - $basearch', ensure => 'present', @@ -13,9 +10,9 @@ gpgkey => 'http://myownmirror', enabled => '1', gpgcheck => '1', - target => '/etc/yum.repo.d/puppetlabs.repo', + target => '/etc/yum.repos.d/puppetlabs.repo', } -yumrepo{'puppetrepo-deps': +yumrepo { 'puppetrepo-deps': name => 'puppetrepo-deps', descr => 'Puppet Labs Dependencies El 7 - $basearch', ensure => 'present', @@ -23,7 +20,7 @@ gpgkey => 'http://myownmirror', enabled => '1', gpgcheck => '1', - target => '/etc/yum.repo.d/puppetlabs.repo', + target => '/etc/yum.repos.d/puppetlabs.repo', } MANIFEST @@ -36,34 +33,31 @@ def resource(host, type, name) RSpec.context 'Manages yumrepo' do agents.each do |agent| - it 'creates multiple yum repo files' do + it 'creates a yum repo file' do apply_manifest_on(agent, manifest) - [products_repo, deps_repo].each do |repo| - resource(agent, 'file', repo) do |res| - assert_equal(res['ensure'], 'file') - assert_equal(res['mode'], '0644') - end + resource(agent, 'file', puppet_repo) do |res| + assert_equal(res['ensure'], 'file') + assert_equal(res['mode'], '0644') end end it 'removes a yumrepo entry' do apply_manifest_on(agent, < 'puppetrepo-deps', ensure => 'absent', - target => '/etc/yum.repo.d/puppetlabs.repo', + target => '/etc/yum.repos.d/puppetlabs.repo', } ABSENT resource(agent, 'file', deps_repo) do |res| - # absent removes the entry, leaving an empty file with a known checksum assert_equal(res['ensure'], 'file') # Puppet 7 and up uses SHA256 as the default digest algorithm if %r{^6\.}.match?(on(agent, puppet('--version')).stdout) - assert_equal(res['content'], '{md5}d41d8cd98f00b204e9800998ecf8427e') + assert_equal(res['content'], '{md5}8df43e112c614f3062545995b32ed3c0') else - assert_equal(res['content'], '{sha256}e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855') + assert_equal(res['content'], '{sha256}a361f9e6174b1f3baca261d254c21d8d89ca274b36ebdfee5bf3223f1820aeea') end end end @@ -71,7 +65,7 @@ def resource(host, type, name) it 'updates a yumrepo entry' do apply_manifest_on(agent, manifest) apply_manifest_on(agent, < 'present', enabled => 'no', baseurl => 'http://myothermirror', diff --git a/spec/unit/provider/yumrepo/inifile_spec.rb b/spec/unit/provider/yumrepo/inifile_spec.rb index a26fe13..7e4073f 100644 --- a/spec/unit/provider/yumrepo/inifile_spec.rb +++ b/spec/unit/provider/yumrepo/inifile_spec.rb @@ -31,11 +31,11 @@ describe 'generating the virtual inifile' do let(:files) { ['/etc/yum.repos.d/first.repo', '/etc/yum.repos.d/second.repo', '/etc/yum.conf'] } - let(:collection) { instance_double('Puppet::Util::IniConfig::FileCollection') } + let(:collection) { instance_double('Puppet::Provider::Yumrepo::IniConfig::FileCollection') } before(:each) do described_class.clear - allow(Puppet::Util::IniConfig::FileCollection).to receive(:new).and_return collection + allow(Puppet::Provider::Yumrepo::IniConfig::FileCollection).to receive(:new).and_return collection end it 'reads all files in the directories specified by self.repofiles' do @@ -63,10 +63,10 @@ end describe 'creating provider instances' do - let(:virtual_inifile) { instance_double('Puppet::Util::IniConfig::FileCollection') } + let(:virtual_inifile) { instance_double('Puppet::Provider::Yumrepo::IniConfig::FileCollection') } let(:main_section) do - sect = Puppet::Util::IniConfig::Section.new('main', '/some/imaginary/file') + sect = Puppet::Provider::Yumrepo::IniConfig::Section.new('main', '/some/imaginary/file') sect.entries << ['distroverpkg', 'centos-release'] sect.entries << ['plugins', '1'] @@ -74,7 +74,7 @@ end let(:updates_section) do - sect = Puppet::Util::IniConfig::Section.new('updates', '/some/imaginary/file') + sect = Puppet::Provider::Yumrepo::IniConfig::Section.new('updates', '/some/imaginary/file') sect.entries << ['name', 'Some long description of the repo'] sect.entries << ['enabled', '1'] @@ -104,44 +104,231 @@ end describe 'retrieving a section from the inifile' do - let(:collection) { instance_double('Puppet::Util::IniConfig::FileCollection') } + let(:collection) { instance_double('Puppet::Provider::Yumrepo::IniConfig::FileCollection') } - let(:ini_section) { instance_double('Puppet::Util::IniConfig::Section') } + let(:ini_section) { instance_double('Puppet::Provider::Yumrepo::IniConfig::Section', name: 'updates') } before(:each) do allow(described_class).to receive(:virtual_inifile).and_return(collection) + allow(described_class).to receive(:reposdir).and_return ['/etc/yum.repos.d'] end describe 'and the requested section exists' do before(:each) do - allow(collection).to receive(:[]).with('updates').and_return ini_section + allow(collection).to receive(:each_section).and_yield(ini_section) end - it 'returns the existing section' do - expect(described_class.section('updates')).to eq ini_section + describe 'with no target specified' do + it 'returns the existing section' do + expect(described_class.section('updates', nil)).to eq ini_section + end + + it "doesn't create a new section" do + expect(collection).to receive(:add_section).never + described_class.section('updates', nil) + end + end + + describe 'with a matching target' do + target = '/etc/yum.repos.d/expected-path.repo' + + let(:ini_section) { instance_double('Puppet::Provider::Yumrepo::IniConfig::Section', name: 'updates', file: target) } + + it 'returns the existing section' do + expect(described_class.section('updates', target)).to eq ini_section + end + + it "doesn't create a new section" do + expect(collection).to receive(:add_section).never + described_class.section('updates', target) + end end - it "doesn't create a new section" do - expect(collection).to receive(:add_section).never - described_class.section('updates') + describe 'with a target that differs from an existing repo file' do + target = '/etc/yum.repos.d/expected-path.repo' + + before(:each) do + allow(collection).to receive(:add_section).with('updates', target).and_return(new_ini_section) + allow(ini_section).to receive(:destroy=).with(true) + allow(ini_section).to receive(:entries).and_return(['enabled', true]) + end + + let(:ini_section) { instance_double('Puppet::Provider::Yumrepo::IniConfig::Section', name: 'updates', file: '/etc/yum.repos.d/updates.repo') } + let(:new_ini_section) { Puppet::Provider::Yumrepo::IniConfig::Section.new('updates', target) } + + it 'creates a new section' do + expect(collection).to receive(:add_section).with('updates', target) + described_class.section('updates', target) + end + + it 'marks old section for destruction' do + expect(ini_section).to receive(:destroy=).with(true) + described_class.section('updates', target) + end + + it 'copies the old section' do + expect(ini_section).to receive(:entries).and_return(['enabled', true]) + section = described_class.section('updates', target) + expect(section.entries).to eq(['enabled', true]) + end end end describe "and the requested section doesn't exist" do - it 'creates a section in the preferred repodir' do + before(:each) do + allow(collection).to receive(:each_section) allow(described_class).to receive(:reposdir).and_return ['/etc/yum.repos.d', '/etc/alternate.repos.d'] - expect(collection).to receive(:[]).with('updates') - expect(collection).to receive(:add_section).with('updates', '/etc/alternate.repos.d/updates.repo') + end + + describe 'with no target specified' do + let(:target) { nil } + + it 'creates a section in the preferred repodir' do + expect(collection).to receive(:add_section).with('updates', '/etc/alternate.repos.d/updates.repo') + + described_class.section('updates', target) + end + + it 'creates a section in yum.conf if no repodirs exist' do + allow(described_class).to receive(:reposdir).and_return [] + expect(collection).to receive(:add_section).with('updates', '/etc/yum.conf') + + described_class.section('updates', target) + end + end + + describe 'with relative path target specified' do + let(:target) { 'custom_file.repo' } + + it 'creates a section in the preferred repodir' do + expect(collection).to receive(:add_section).with('updates', "/etc/alternate.repos.d/#{target}") + + described_class.section('updates', target) + end + + it 'creates a section in yum.conf if no repodirs exist' do + allow(described_class).to receive(:reposdir).and_return [] + expect(collection).to receive(:add_section).with('updates', '/etc/yum.conf') + + described_class.section('updates', target) + end + end + + describe 'with absolute path target specified' do + describe 'path is not in valid repodir' do + let(:target) { '/nonexistent_dir/custom_file.repo' } + + it 'creates a section in the preferred repodir' do + expect(collection).to receive(:add_section).with('updates', '/etc/alternate.repos.d/custom_file.repo') + + described_class.section('updates', target) + end + + it 'creates a section in yum.conf if no repodirs exist' do + allow(described_class).to receive(:reposdir).and_return [] + expect(collection).to receive(:add_section).with('updates', '/etc/yum.conf') + + described_class.section('updates', target) + end + end + describe 'path is in valid repodir' do + let(:target) { '/etc/yum.repos.d/custom_file.repo' } + + it 'creates a section in the preferred repodir' do + expect(collection).to receive(:add_section).with('updates', target) + + described_class.section('updates', target) + end + + it 'creates a section in yum.conf if no repodirs exist' do + allow(described_class).to receive(:reposdir).and_return [] + expect(collection).to receive(:add_section).with('updates', '/etc/yum.conf') + + described_class.section('updates', target) + end + end + end + end + end + + describe 'repo_path' do + subject(:repo_path) { described_class.repo_path(name, target) } + + before(:each) do + allow(described_class).to receive(:reposdir).and_return ['/etc/yum.repos.d'] + end + + let(:name) { 'updates' } + + describe 'target is nil' do + let(:target) { nil } + + it 'uses repo name in default directory' do + expect(repo_path).to eq('/etc/yum.repos.d/updates.repo') + end + end + + describe 'target is :absent' do + let(:target) { :absent } + + it 'uses repo name in default directory' do + expect(repo_path).to eq('/etc/yum.repos.d/updates.repo') + end + end + + describe 'target path is absolute' do + describe 'in an invalid repodir' do + describe 'with .repo suffix' do + let(:target) { '/nonexistent_dir/custom_file.repo' } + + it 'uses target basename in default directory' do + expect(repo_path).to eq('/etc/yum.repos.d/custom_file.repo') + end + end + + describe 'without .repo suffix' do + let(:target) { '/nonexistent_dir/custom_file' } + + it 'uses target basename in default directory' do + expect(repo_path).to eq('/etc/yum.repos.d/custom_file.repo') + end + end + end + + describe 'in a valid repodir' do + describe 'with .repo suffix' do + let(:target) { '/etc/yum.repos.d/custom_file.repo' } + + it 'uses target' do + expect(repo_path).to eq('/etc/yum.repos.d/custom_file.repo') + end + end + + describe 'without .repo suffix' do + let(:target) { '/etc/yum.repos.d/custom_file' } + + it 'uses target' do + expect(repo_path).to eq('/etc/yum.repos.d/custom_file.repo') + end + end + end + end + + describe 'target path is relative' do + describe 'with .repo suffix' do + let(:target) { 'custom_file.repo' } - described_class.section('updates') + it 'uses target in default directory' do + expect(repo_path).to eq('/etc/yum.repos.d/custom_file.repo') + end end - it 'creates a section in yum.conf if no repodirs exist' do - allow(described_class).to receive(:reposdir).and_return [] - expect(collection).to receive(:[]).with('updates') - expect(collection).to receive(:add_section).with('updates', '/etc/yum.conf') + describe 'without .repo suffix' do + let(:target) { 'custom_file' } - described_class.section('updates') + it 'uses target in default directory' do + expect(repo_path).to eq('/etc/yum.repos.d/custom_file.repo') + end end end end @@ -164,12 +351,13 @@ end let(:section) do - instance_double('Puppet::Util::IniConfig::Section', name: 'puppetlabs-products') + instance_double('Puppet::Provider::Yumrepo::IniConfig::Section', name: 'puppetlabs-products') end before(:each) do type_instance.provider = provider - allow(described_class).to receive(:section).with('puppetlabs-products').and_return(section) + allow(described_class).to receive(:reposdir).and_return ['/etc/yum.repos.d'] + allow(described_class).to receive(:section).with('puppetlabs-products', '/etc/yum.repos.d/puppetlabs-products.repo').and_return(section) end describe 'methods used by ensurable' do @@ -184,7 +372,7 @@ end it '#exists? checks if the repo has been marked as present' do - allow(described_class).to receive(:section).and_return(instance_double('Puppet::Util::IniConfig::Section', :[]= => nil)) + allow(described_class).to receive(:section).and_return(instance_double('Puppet::Provider::Yumrepo::IniConfig::Section', :[]= => nil)) provider.create expect(provider).to be_exist end @@ -292,12 +480,12 @@ end describe 'and the file exists' do - let(:pfile) { instance_double('Puppet::Util::IniConfig::PhysicalFile') } - let(:sect) { instance_double('Puppet::Util::IniConfig::Section') } + let(:pfile) { instance_double('Puppet::Provider::Yumrepo::IniConfig::PhysicalFile') } + let(:sect) { instance_double('Puppet::Provider::Yumrepo::IniConfig::Section') } before(:each) do allow(Puppet::FileSystem).to receive(:exist?).with('/etc/yum.conf').and_return true - allow(Puppet::Util::IniConfig::PhysicalFile).to receive(:new).with('/etc/yum.conf').and_return pfile + allow(Puppet::Provider::Yumrepo::IniConfig::PhysicalFile).to receive(:new).with('/etc/yum.conf').and_return pfile allow(pfile).to receive(:read) end From d5ed15a673447edf6c9b096fb043651ac12c8e2c Mon Sep 17 00:00:00 2001 From: Michael Hashizume Date: Thu, 1 Jun 2023 13:25:54 -0700 Subject: [PATCH 2/2] (maint) Update commit check This commit updates the commits Rake check to include commit summaries that begin with the Puppet Agent (PA) Jira project and removes references to a non-existent CONTRIBUTION.md. --- rakelib/commits.rake | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/rakelib/commits.rake b/rakelib/commits.rake index 42eb209..7cee9fa 100644 --- a/rakelib/commits.rake +++ b/rakelib/commits.rake @@ -1,4 +1,4 @@ -desc "verify that commit messages match CONTRIBUTING.md requirements" +desc "verify that commit summaries are properly formatted" task(:commits) do # This rake task looks at the summary from every commit from this branch not # in the branch targeted for a PR. @@ -7,11 +7,10 @@ task(:commits) do %x{git log --no-merges --pretty=%s #{commit_range}}.each_line do |commit_summary| # This regex tests for the currently supported commit summary tokens. # The exception tries to explain it in more full. - if /^Release prep|\((maint|packaging|doc|docs|modules-\d+)\)|revert/i.match(commit_summary).nil? - raise "\n\n\n\tThis commit summary didn't match CONTRIBUTING.md guidelines:\n" \ - "\n\t\t#{commit_summary}\n" \ - "\tThe commit summary (i.e. the first line of the commit message) should start with one of:\n" \ - "\t\t(MODULES-) # this is most common and should be a ticket at tickets.puppet.com\n" \ + if /^Release prep|\((maint|packaging|doc|docs|modules|pa-\d+)\)|revert/i.match(commit_summary).nil? + raise "\n\n\n\tPlease make sure that your commit summary (i.e. the first line of the commit message) starts with one of the following:\n" \ + "\t\t(PA-)\n" \ + "\t\t(MODULES-)\n" \ "\t\t(docs)\n" \ "\t\t(docs)(DOCUMENT-)\n" \ "\t\t(packaging)\n"