Merge pull request #54 from mhashizume/PA-5187/main/target

(PA-5187) Add target support

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

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

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-<digits>) # 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-<digits>)\n" \
+        "\t\t(MODULES-<digits>)\n" \
         "\t\t(docs)\n" \
         "\t\t(docs)(DOCUMENT-<digits>)\n" \
         "\t\t(packaging)\n"

spec/acceptance/tests/yumrepo_spec.rb

@@ -1,29 +1,26 @@
 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 = <<MANIFEST
-yumrepo {'puppetrepo-products':
+yumrepo { 'puppetrepo-products':
   name      => 'puppetrepo-products',
   descr     => 'Puppet Labs Products El 7 - $basearch',
   ensure    => 'present',
   baseurl   => 'http://myownmirror',
   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',
   baseurl   => 'http://myownmirror',
   gpgkey    => 'http://myownmirror',
   enabled   => '1',
   gpgcheck  => '1',
-  target    => '/etc/yum.repo.d/puppetlabs.repo',
+  target    => '/etc/yum.repos.d/puppetlabs.repo',
 }
 MANIFEST
 
@@ -36,42 +33,39 @@ 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, <<ABSENT)
-yumrepo{'puppetrepo-deps':
+yumrepo { 'puppetrepo-deps':
   name      => '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
 
     it 'updates a yumrepo entry' do
       apply_manifest_on(agent, manifest)
       apply_manifest_on(agent, <<UPDATED)
-yumrepo {'puppetrepo-products':
+yumrepo { 'puppetrepo-products':
   ensure    => 'present',
   enabled   => 'no',
   baseurl   => 'http://myothermirror',