Add insert_type and subsetting_key_val_separator

* Insert_type and insert value can define where
  a new subsetting element will be placed.
* subsetting_key_val_separator allows you to have
  a separator character between a subsetting name
  and its value.
* Refactor ini_subsetting and its spec.

README.markdown

@@ -409,6 +409,10 @@ Global show_diff configuraton takes priority over this one -
 
 *Optional.* Specifies a string to use between subsettings. Valid options: a string. Default value: " ".
 
+##### `subsetting_key_val_separator`
+
+*Optional.* Specifies a string to use between subsetting name and value (if there is a separator between the subsetting name and its value). Valid options: a string. Default value: empty string.
+
 ##### `use_exact_match`
 
 *Optional.* Whether to use partial or exact matching for subsetting. Should be set to true if the subsettings do not have values. Valid options: true, false. Default value: false.
@@ -417,6 +421,20 @@ Global show_diff configuraton takes priority over this one -
 
 *Optional.* Supplies a value for the specified subsetting. Valid options: a string. Default value: undefined.
 
+##### `insert_type`
+
+*Optional.* Selects where a new subsetting item should be inserted.
+
+* *start*  - insert at the beginning of the line.
+* *end*    - insert at the end of the line (default).
+* *before* - insert before the specified element if possible.
+* *after*  - insert after the specified element if possible.
+* *index*  - insert at the specified index number.
+
+##### `insert_value`
+
+*Optional.* The value for the insert type if the value if required.
+
 ### Function: create_ini_settings
 
 Manages multiple `ini_setting` resources from a hash. Note that this cannot be used with ini_subsettings.

examples/ini_subsetting.pp

@@ -1,5 +1,5 @@
 ini_subsetting { 'sample subsetting':
-  ensure            => present,
+  ensure            => 'present',
   section           => '',
   key_val_separator => '=',
   path              => '/etc/default/pe-puppetdb',
@@ -9,10 +9,23 @@ ini_subsetting { 'sample subsetting':
 }
 
 ini_subsetting { 'sample subsetting2':
-  ensure            => absent,
+  ensure            => 'absent',
   section           => '',
   key_val_separator => '=',
   path              => '/etc/default/pe-puppetdb',
   setting           => 'JAVA_ARGS',
   subsetting        => '-Xms',
 }
+
+ini_subsetting { 'sample subsetting3':
+  ensure                       => 'present',
+  section                      => '',
+  key_val_separator            => '=',
+  subsetting_key_val_separator => ':',
+  path                         => '/etc/default/pe-puppetdb',
+  setting                      => 'JAVA_ARGS',
+  subsetting                   => '-XX',
+  value                        => '+HeapDumpOnOutOfMemoryError',
+  insert_type                  => 'after',
+  insert_value                 => '-Xmx',
+}

lib/puppet/provider/ini_subsetting/ruby.rb

@@ -8,7 +8,10 @@ Puppet::Type.type(:ini_subsetting).provide(:ruby) do
   end
 
   def create
-    setting_value.add_subsetting(subsetting, resource[:value], resource[:use_exact_match])
+    setting_value.add_subsetting(
+        subsetting, resource[:value], resource[:use_exact_match],
+        resource[:insert_type], resource[:insert_value]
+    )
     ini_file.set_value(section, setting, setting_value.get_value)
     ini_file.save
     @ini_file = nil
@@ -28,7 +31,10 @@ Puppet::Type.type(:ini_subsetting).provide(:ruby) do
   end
 
   def value=(value)
-    setting_value.add_subsetting(subsetting, resource[:value], resource[:use_exact_match])
+    setting_value.add_subsetting(
+        subsetting, value, resource[:use_exact_match],
+        resource[:insert_type], resource[:insert_value]
+    )
     ini_file.set_value(section, setting, setting_value.get_value)
     ini_file.save
   end
@@ -53,21 +59,28 @@ Puppet::Type.type(:ini_subsetting).provide(:ruby) do
     resource[:path]
   end
 
-  def separator
+  def key_val_separator
     resource[:key_val_separator] || '='
   end
 
+  def subsetting_key_val_separator
+    resource[:subsetting_key_val_separator] || ''
+  end
+
   def quote_char
     resource[:quote_char]
   end
 
   private
   def ini_file
-    @ini_file ||= Puppet::Util::IniFile.new(file_path, separator)
+    @ini_file ||= Puppet::Util::IniFile.new(file_path, key_val_separator)
   end
 
   def setting_value
-    @setting_value ||= Puppet::Util::SettingValue.new(ini_file.get_value(section, setting), subsetting_separator, quote_char)
+    @setting_value ||= Puppet::Util::SettingValue.new(
+        ini_file.get_value(section, setting),
+        subsetting_separator, quote_char, subsetting_key_val_separator
+    )
   end
 
 end

lib/puppet/type/ini_subsetting.rb

@@ -43,6 +43,11 @@ Puppet::Type.newtype(:ini_subsetting) do
     defaultto(" ")
   end
 
+  newparam(:subsetting_key_val_separator) do
+    desc 'The separator string between the subsetting name and its value. Defaults to the empty string.'
+    defaultto('')
+  end
+
   newparam(:path) do
     desc 'The ini file Puppet will ensure contains the specified setting.'
     validate do |value|
@@ -109,4 +114,23 @@ Puppet::Type.newtype(:ini_subsetting) do
     end
   end
 
+  newparam(:insert_type) do
+    desc <<-eof
+Where the new subsetting item should be inserted?
+
+* :start  - insert at the beginning of the line.
+* :end    - insert at the end of the line (default).
+* :before - insert before the specified element if possible.
+* :after  - insert after the specified element if possible.
+* :index  - insert at the specified index number.
+    eof
+
+    newvalues(:start, :end, :before, :after, :index)
+    defaultto(:end)
+  end
+
+  newparam(:insert_value) do
+    desc 'The value for the insert types which require one.'
+  end
+
 end

lib/puppet/util/setting_value.rb

@@ -1,103 +1,187 @@
 module Puppet
   module Util
-
+    # This class can work with a list of subsettings inside
+    # an ini file setting string to add, remove, extract and set their values.
     class SettingValue
 
-      def initialize(setting_value, subsetting_separator = ' ', default_quote_char = nil)
+      # The constructor method
+      # @param setting_value [String] The initial setting value
+      # @param subsetting_separator [String] The character is used to separate
+      # subsettings in the setting_value string.
+      # @param default_quote_char [String] Quote the setting string with this character.
+      def initialize(setting_value, subsetting_separator = ' ', default_quote_char = '', key_val_separator = '')
         @setting_value = setting_value
         @subsetting_separator = subsetting_separator
-
-        default_quote_char ||= ''
+        @quote_char = default_quote_char
+        @key_val_separator = key_val_separator
+        @subsetting_items = []
 
         if @setting_value
-          unquoted, @quote_char = unquote_setting_value(setting_value)
-          @subsetting_items = unquoted.scan(Regexp.new("(?:(?:[^\\#{@subsetting_separator}]|\\.)+)"))  # an item can contain escaped separator
+          unquoted, quote_char = unquote_setting_value(setting_value)
+          @quote_char = quote_char unless quote_char.empty?
+          # an item can contain escaped separator
+          @subsetting_items = unquoted.scan(Regexp.new("(?:(?:[^\\#{@subsetting_separator}]|\\.)+)"))
           @subsetting_items.map! { |item| item.strip }
-          @quote_char = default_quote_char if @quote_char.empty?
-        else
-          @subsetting_items = []
-          @quote_char = default_quote_char
         end
       end
 
+      # If the setting value is quoted, the quotes are
+      # removed and the unquoted string and the quoting
+      # character are returned.
+      # @param setting_value [String] The input value
+      # @return [Array] The unquoted string and the quoting character
       def unquote_setting_value(setting_value)
-        quote_char = ""
-        if (setting_value.start_with?('"') and setting_value.end_with?('"'))
+        quote_char = ''
+        if setting_value.start_with?('"') and setting_value.end_with?('"')
           quote_char = '"'
-        elsif (setting_value.start_with?("'") and setting_value.end_with?("'"))
+        elsif setting_value.start_with?("'") and setting_value.end_with?("'")
           quote_char = "'"
         end
 
-        unquoted = setting_value
-
-        if (quote_char != "")
+        if quote_char != ''
           unquoted = setting_value[1, setting_value.length - 2]
+        else
+          unquoted = setting_value
         end
 
         [unquoted, quote_char]
       end
 
+      # Get the resulting setting value by joining all the
+      # subsettings, separator and quote characters.
+      # @return [String]
       def get_value
-
-        result = ""
-        first = true
-
-        @subsetting_items.each { |item|
-          result << @subsetting_separator unless first
-          result << item
-          first = false
-        }
-
-        @quote_char + result + @quote_char
+        value = @subsetting_items.join @subsetting_separator
+        @quote_char + value + @quote_char
       end
 
+      # Get the value of the given subsetting item.
+      # If the exact match is used the value will be true
+      # if the item is found.
+      # @param subsetting [String] The name of the subsetting to add.
+      # @param use_exact_match [:true,:false] Should the full name match be used?
+      # @return [nil,true,String]
       def get_subsetting_value(subsetting, use_exact_match=:false)
-
-        value = nil
-
-        @subsetting_items.each { |item|
-          if(use_exact_match == :false and item.start_with?(subsetting))
-            value = item[subsetting.length, item.length - subsetting.length]
-            break
-          elsif(use_exact_match == :true and item.eql?(subsetting))
-            return true
-          end
-        }
-
-        value
+        index = find_subsetting(subsetting, use_exact_match)
+        # the item is not found in the list
+        return nil unless index
+        # the exact match is set and the item is found, the value should be true
+        return true if use_exact_match == :true
+        item = @subsetting_items[index]
+        item[(subsetting.length + @key_val_separator.length)..-1]
       end
 
-      def add_subsetting(subsetting, subsetting_value, use_exact_match=:false)
+      # Add a new subsetting item to the list of existing items
+      # if such item is not already there.
+      # @param subsetting [String] The name of the subsetting to add.
+      # @param subsetting_value [String] The value of the subsetting.
+      # It will be appended to the name.
+      # @param use_exact_match [:true,:false] Should the full name match be used?
+      # @param [Symbol] insert_type
+      # @param [String,Integer] insert_value
+      # @return [Array] The resulting subsettings list.
+      def add_subsetting(subsetting, subsetting_value, use_exact_match=:false, insert_type=:end, insert_value=nil)
+        index = find_subsetting(subsetting, use_exact_match)
+
+        # update the existing values if the subsetting is found in the list
+        return update_subsetting(subsetting, subsetting_value, use_exact_match) if index
+
+        new_item = item_value(subsetting, subsetting_value)
+
+        case insert_type
+          when :start
+            @subsetting_items.unshift(new_item)
+          when :end
+            @subsetting_items.push(new_item)
+          when :before
+            before_index = find_subsetting(insert_value, use_exact_match)
+            if before_index
+              @subsetting_items.insert(before_index, new_item)
+            else
+              @subsetting_items.push(new_item)
+            end
+          when :after
+            after_index = find_subsetting(insert_value, use_exact_match)
+            if after_index
+              @subsetting_items.insert(after_index + 1, new_item)
+            else
+              @subsetting_items.push(new_item)
+            end
+          when :index
+            before_index = insert_value.to_i
+            before_index = @subsetting_items.length if before_index > @subsetting_items.length
+            @subsetting_items.insert(before_index, new_item)
+          else
+            @subsetting_items.push(new_item)
+        end
 
-        new_item = subsetting + (subsetting_value || '')
-        found = false
+        @subsetting_items
+      end
 
-        @subsetting_items.map! { |item|
-          if use_exact_match == :false and item.start_with?(subsetting)
-            value = new_item
-            found = true
-          elsif use_exact_match == :true and item.eql?(subsetting)
-            value = new_item
-            found = true
+      # Update all matching items in the settings list to the new values.
+      # @param subsetting [String] The name of the subsetting to add.
+      # @param subsetting_value [String] The value of the subsetting.
+      # @param use_exact_match [:true,:false] Should the full name match be used?
+      # @return [Array] The resulting subsettings list.
+      def update_subsetting(subsetting, subsetting_value, use_exact_match=:false)
+        new_item = item_value(subsetting, subsetting_value)
+        @subsetting_items.map! do |item|
+          if match_subsetting?(item, subsetting, use_exact_match)
+            new_item
           else
-            value = item
+            item
           end
+        end
+      end
 
-          value
-        }
+      # Find the first subsetting item matching the given name,
+      # or, if the exact match is set, equal to the given name
+      # and return its array index value. Returns nil if not found.
+      # @param subsetting [String] The name of the subsetting to search.
+      # @param use_exact_match [:true,:false] Look for the full string match?
+      # @return [Integer, nil]
+      def find_subsetting(subsetting, use_exact_match=:false)
+        @subsetting_items.index do |item|
+          match_subsetting?(item, subsetting, use_exact_match)
+        end
+      end
 
-        unless found
-          @subsetting_items.push(new_item)
+      # Check if the subsetting item matches the given name.
+      # If the exact match is set the entire item is matched,
+      # and only the item name and separator string if not.
+      # @param item [String] The item value to check against the subsetting name.
+      # @param subsetting [String] The subsetting name.
+      # @param use_exact_match [:true,:false] Look for the full string match?
+      # @return [true,false]
+      def match_subsetting?(item, subsetting, use_exact_match=:false)
+        if use_exact_match == :true
+          item.eql?(subsetting)
+        else
+          item.start_with?(subsetting + @key_val_separator)
         end
       end
 
+      # Remove all the subsetting items that match
+      # the given subsetting name.
+      # @param subsetting [String] The subsetting name to remove.
+      # @param use_exact_match [:true,:false] Look for the full string match?
+      # @return [Array] The resulting subsettings list.
       def remove_subsetting(subsetting, use_exact_match=:false)
-        if use_exact_match == :false
-          @subsetting_items = @subsetting_items.map { |item| item.start_with?(subsetting) ? nil : item }.compact
-        else
-          @subsetting_items = @subsetting_items.map { |item| item.eql?(subsetting) ? nil : item }.compact
+        @subsetting_items.delete_if do |item|
+          match_subsetting?(item, subsetting, use_exact_match)
         end
       end
+
+      # The actual value of the subsetting item.
+      # It's built from the subsetting name, its value and the separator
+      # string if present.
+      # @param subsetting [String] The subsetting name
+      # @param subsetting_value [String] The value of the subsetting
+      # @return [String]
+      def item_value(subsetting, subsetting_value)
+        (subsetting || '') + (@key_val_separator || '') + (subsetting_value || '')
+      end
+
     end
   end
 end

spec/acceptance/ini_subsetting_spec.rb

@@ -153,6 +153,40 @@ describe 'ini_subsetting resource' do
     end
   end
 
+  describe 'subsetting_key_val_separator' do
+    {
+        ''                                        => /two = twinethree foobar/,
+        "subsetting_key_val_separator => ':',"    => /two = twine:three foo:bar/,
+        "subsetting_key_val_separator => '-',"    => /two = twine-three foo-bar/,
+    }.each do |parameter, content|
+      context "with '#{parameter}' makes '#{content}'" do
+        pp = <<-EOS
+        ini_subsetting { "with #{parameter} makes #{content}":
+          ensure     => 'present',
+          section    => 'one',
+          setting    => 'two',
+          subsetting => 'twine',
+          value      => 'three',
+          path       => "#{tmpdir}/subsetting_key_val_separator.ini",
+          before     => Ini_subsetting['foobar'],
+          #{parameter}
+        }
+        ini_subsetting { "foobar":
+          ensure     => 'present',
+          section    => 'one',
+          setting    => 'two',
+          subsetting => 'foo',
+          value      => 'bar',
+          path       => "#{tmpdir}/subsetting_key_val_separator.ini",
+          #{parameter}
+        }
+        EOS
+
+        it_behaves_like 'has_content', "#{tmpdir}/subsetting_key_val_separator.ini", pp, content
+      end
+    end
+  end
+
   describe 'quote_char' do
     {
         ['-Xmx']         => /args=""/,
@@ -234,4 +268,69 @@ describe 'ini_subsetting resource' do
     end
   end
 
+  describe 'insert types:' do
+    [
+        {
+            :insert_type => :start,
+            :content     => /d a b c/,
+        },
+        {
+            :insert_type => :end,
+            :content     => /a b c d/,
+        },
+        {
+            :insert_type  => :before,
+            :insert_value => 'c',
+            :content      => /a b d c/,
+        },
+        {
+            :insert_type  => :after,
+            :insert_value => 'a',
+            :content      => /a d b c/,
+        },
+        {
+            :insert_type  => :index,
+            :insert_value => 2,
+            :content      => /a b d c/,
+        }
+    ].each do |params|
+      context "with '#{params[:insert_type]}' makes '#{params[:content]}'" do
+        pp = <<-EOS
+        ini_subsetting { "a":
+          ensure     => present,
+          section    => 'one',
+          setting    => 'two',
+          subsetting => 'a',
+          path       => "#{tmpdir}/insert_types.ini",
+        } ->
+        ini_subsetting { "b":
+          ensure     => present,
+          section    => 'one',
+          setting    => 'two',
+          subsetting => 'b',
+          path       => "#{tmpdir}/insert_types.ini",
+        } ->
+        ini_subsetting { "c":
+          ensure     => present,
+          section    => 'one',
+          setting    => 'two',
+          subsetting => 'c',
+          path       => "#{tmpdir}/insert_types.ini",
+        } ->
+        ini_subsetting { "insert makes #{params[:content]}":
+          ensure       => present,
+          section      => 'one',
+          setting      => 'two',
+          subsetting   => 'd',
+          path         => "#{tmpdir}/insert_types.ini",
+          insert_type  => '#{params[:insert_type]}',
+          insert_value => '#{params[:insert_value]}',
+        }
+        EOS
+
+        it_behaves_like 'has_content', "#{tmpdir}/insert_types.ini", pp, params[:content]
+      end
+    end
+  end
+
 end

spec/unit/puppet/provider/ini_subsetting/ruby_spec.rb

@@ -8,10 +8,9 @@ describe provider_class do
   let(:tmpfile) { tmpfilename("ini_setting_test") }
 
   def validate_file(expected_content, tmpfile)
-    File.read(tmpfile).should == expected_content
+    expect(File.read(tmpfile)).to eq expected_content
   end
 
-
   before :each do
     File.open(tmpfile, 'w') do |fh|
       fh.write(orig_content)
@@ -37,7 +36,7 @@ JAVA_ARGS="-Xmx192m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe
       resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
          :subsetting => '-Xms', :value => '128m'))
       provider = described_class.new(resource)
-      provider.exists?.should be_nil
+      expect(provider.exists?).to be_nil
       provider.create
       expected_content = <<-EOS
 JAVA_ARGS="-Xmx192m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof -Xms128m"
@@ -45,29 +44,113 @@ JAVA_ARGS="-Xmx192m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe
       validate_file(expected_content, tmpfile)
     end
 
+    it 'should add a missing subsetting element at the beginning of the line' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-Xms', :value => '128m', :insert_type => :start))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be_nil
+      provider.create
+      expected_content = <<-EOS
+JAVA_ARGS="-Xms128m -Xmx192m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it 'should add a missing subsetting element at the end of the line' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-Xms', :value => '128m', :insert_type => :end))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be_nil
+      provider.create
+      expected_content = <<-EOS
+JAVA_ARGS="-Xmx192m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof -Xms128m"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it 'should add a missing subsetting element after the given item' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-Xms', :value => '128m', :insert_type => :after, :insert_value => '-Xmx'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be_nil
+      provider.create
+      expected_content = <<-EOS
+JAVA_ARGS="-Xmx192m -Xms128m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it 'should add a missing subsetting element before the given item' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-Xms', :value => '128m', :insert_type => :before, :insert_value => '-Xmx'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be_nil
+      provider.create
+      expected_content = <<-EOS
+JAVA_ARGS="-Xms128m -Xmx192m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it 'should add a missing subsetting element at the given index' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-Xms', :value => '128m', :insert_type => :index, :insert_value => '1'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be_nil
+      provider.create
+      expected_content = <<-EOS
+JAVA_ARGS="-Xmx192m -Xms128m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
     it "should remove an existing subsetting" do
       resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
           :subsetting => '-Xmx'))
       provider = described_class.new(resource)
-      provider.exists?.should == "192m"
+      expect(provider.exists?).to eq '192m'
       provider.destroy
       expected_content = <<-EOS
 JAVA_ARGS="-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof"
       EOS
       validate_file(expected_content, tmpfile)
     end
+
+    it 'should be able to remove several subsettings with the same name' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-XX'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to eq ':+HeapDumpOnOutOfMemoryError'
+      provider.destroy
+      expected_content = <<-EOS
+JAVA_ARGS="-Xmx192m"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
     
     it "should modify an existing subsetting" do
       resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
           :subsetting => '-Xmx', :value => '256m'))
       provider = described_class.new(resource)
-      provider.exists?.should == "192m"
+      expect(provider.exists?).to eq '192m'
       provider.value=('256m')
       expected_content = <<-EOS
 JAVA_ARGS="-Xmx256m -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/var/log/pe-puppetdb/puppetdb-oom.hprof"
       EOS
       validate_file(expected_content, tmpfile)
     end
+
+    it 'should be able to modify several subsettings with the same name' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => '-XX', :value => 'test'))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to eq ':+HeapDumpOnOutOfMemoryError'
+      provider.value=('test')
+      expected_content = <<-EOS
+JAVA_ARGS="-Xmx192m -XXtest -XXtest"
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
   end
 
   context "when working with subsettings in files with unquoted settings values" do
@@ -90,7 +173,7 @@ reports = http,foo
       resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
           :subsetting => 'http', :subsetting_separator => ','))
       provider = described_class.new(resource)
-      provider.exists?.should == ""
+      expect(provider.exists?).to eq ''
       provider.destroy
       expected_content = <<-EOS
 [master]
@@ -104,7 +187,7 @@ reports = foo
       resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
           :subsetting => 'puppetdb', :subsetting_separator => ','))
       provider = described_class.new(resource)
-      provider.exists?.should be_nil
+      expect(provider.exists?).to be_nil
       provider.value=('')
       expected_content = <<-EOS
 [master]
@@ -120,7 +203,7 @@ reports = http,foo,puppetdb
           :subsetting => 'puppetdb',
           :subsetting_separator => ','))
       provider = described_class.new(resource)
-      provider.exists?.should be_nil
+      expect(provider.exists?).to be_nil
       provider.value=('')
       expected_content = <<-EOS
 [master]
@@ -178,4 +261,88 @@ reports = http,foo
     end
   end
 
+  context 'when working with subsettings in files with subsetting_key_val_separator' do
+    let(:common_params) { {
+        :title                        => 'ini_setting_ensure_present_test',
+        :path                         => tmpfile,
+        :section                      => 'master',
+        :setting                      => 'reports',
+        :subsetting_separator         => ',',
+        :subsetting_key_val_separator => ':',
+    } }
+
+    let(:orig_content) {
+      <<-EOS
+[master]
+
+reports = a:1,b:2
+      EOS
+    }
+
+    it "should add a new subsetting when the 'parent' setting already exists" do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => 'c',
+          :value      => '3',
+      ))
+      provider = described_class.new(resource)
+      provider.value=('3')
+      expected_content = <<-EOS
+[master]
+
+reports = a:1,b:2,c:3
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it "should add a new subsetting when the 'parent' setting does not already exist" do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => 'c',
+          :value      => '3',
+          :setting    => 'somenewsetting',
+      ))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to be_nil
+      provider.value=('3')
+      expected_content = <<-EOS
+[master]
+
+reports = a:1,b:2
+somenewsetting = c:3
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it 'should be able to remove the existing subsetting' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => 'b',
+      ))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to eq '2'
+      provider.destroy
+      expected_content = <<-EOS
+[master]
+
+reports = a:1
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+    it 'should be able to modify the existing subsetting' do
+      resource = Puppet::Type::Ini_subsetting.new(common_params.merge(
+          :subsetting => 'b',
+          :value      => '5',
+      ))
+      provider = described_class.new(resource)
+      expect(provider.exists?).to eq '2'
+      provider.value=('5')
+      expected_content = <<-EOS
+[master]
+
+reports = a:1,b:5
+      EOS
+      validate_file(expected_content, tmpfile)
+    end
+
+  end
+
 end