Dangling section headers persist in INI files when sections contain whitespace

[peytonw1]

Describe the Bug

When removing the only setting in a section, the section header still remains when the section contains whitespace. As a result, trailing section headers (i.e. sections without any settings) will persist in INI files like below and eventually cause unnecessary clutter:

# cat /tmp/test.ini
[section1]

[section2]

Expected Behavior

We would expect to see section headers to be removed when we are removing the last and only setting in a section.

Steps to Reproduce

Here is an example of a section with a singular setting, in which the empty section header still remains upon ensuring the setting is absent:

1. We created two files (/tmp/test.ini, /tmp/test_remove_setting.pp) as shown below, in which we aimed to remove setting1 and the [section1] header:

# cat /tmp/test.ini
[section1]
setting1 = value1

[section2]
setting2 = value2

# cat /tmp/test_remove_setting.pp
ini_setting { 'remove setting1':
  ensure  => 'absent',
  path    => '/tmp/test.ini',
  section => 'section1',
  setting => 'setting1',
}

2. Applied Puppet:

# puppet apply /tmp/test_remove_setting.pp
Notice: Compiled catalog for <redacted> in environment production in 0.09 seconds
Notice: /Stage[main]/Main/Ini_setting[remove setting1]/ensure: removed
Notice: Applied catalog in 0.69 seconds

3. Observe that while setting1 was removed, the [section1] header still persisted:

# cat /tmp/test.ini
[section1]

[section2]
setting2 = value2

However, when removing the newline after setting1, both the setting AND section header are removed correctly:

4. Modified test.ini to not include a newline after the first setting:

# cat /tmp/test.ini
[section1]
setting1 = value1
[section2]
setting2 = value2

5. Applied Puppet:

# puppet apply /tmp/test_remove_setting.pp
Notice: Compiled catalog for <redacted> in environment production in 0.04 seconds
Notice: /Stage[main]/Main/Ini_setting[remove setting1]/ensure: removed
Notice: Applied catalog in 0.28 seconds

6. Observe that both setting1 and the [section1] header are removed as expected:

# cat /tmp/test.ini
[section2]
setting2 = value2

Environment

• Version: 7.14.0
• Platform: Ubuntu 20.04.1

Additional Context

We also stepped through the puppetlabs-inifile code with pry to observe the IniFile::Section object in the reproduced problem (steps 1-3 above). Initially, we saw that section1 consisted of @start_line=0 and @end_line=2.

[4] pry(#<Puppet::Util::IniFile>)> section
=> #<Puppet::Util::IniFile::Section:<redacted> @additional_settings={}, @end_line=2, @existing_settings={"setting1"=>"value1"}, @indentation=0, @name="section1", @start_line=0>

The start and end lines correspond to the lines with the [section1] header and the newline before the [section2] header, respectively, in the test.ini file:

[5] pry(#<Puppet::Util::IniFile>)> @lines
=> ["[section1]\n", "setting1 = value1\n", "\n", "[section2]\n", "setting2 = value2\n", "\n"]

After removing setting1, we observed in remove_setting() that the section was not registered as empty.

[11] pry(#<Puppet::Util::IniFile>)> next

From: /opt/puppetlabs/puppet/cache/lib/puppet/util/ini_file.rb:137 Puppet::Util::IniFile#remove_setting:

    120: def remove_setting(section_name, setting)
    121:   section = @sections_hash[section_name]
    122:   return unless section.existing_setting?(setting)
    123:   # If the setting is found, we have some work to do.
    124:   # First, we remove the line from our array of lines:
    125:   remove_line(section, setting)
    126:
    127:   # Then, we need to tell the setting object to remove
    128:   # the setting from its state:
    129:   section.remove_existing_setting(setting)
    130:
    131:   # Finally, we need to update all of the start/end line
    132:   # numbers for all of the sections *after* the one that
    133:   # was modified.
    134:   section_index = @section_names.index(section_name)
    135:   decrement_section_line_numbers(section_index + 1)
    136:
 => 137:   return unless section.empty?
    138:   # By convention, it's time to remove this newly emptied out section
    139:   lines.delete_at(section.start_line)
    140:   decrement_section_line_numbers(section_index + 1)
    141:   @section_names.delete_at(section_index)
    142:   @sections_hash.delete(section.name)
    143: end

[12] pry(#<Puppet::Util::IniFile>)> section.empty?
=> false

However, looking at the final state of the Puppet::Util::IniFile::Section object, we saw that section1 now had @start_line=0 and @end_line=1.

[11] pry(#<Puppet::Util::IniFile>)> section
=> #<Puppet::Util::IniFile::Section:<redacted> @additional_settings={}, @end_line=1, @existing_settings={}, @indentation=0, @name="section1", @start_line=0>

[13] pry(#<Puppet::Util::IniFile>)> @lines
=> ["[section1]\n", "\n", "[section2]\n", "setting2 = value2\n", "\n"]

Furthermore, in the section.rb code here, the section.empty? function returns true if the section's start_line == end_line. Thus, this shows that sections containing whitespace are not getting removed correctly, as the code counts the whitespace in the start and end lines to determine if the section is empty.

[smortex]

We would expect to see section headers to be removed when we are removing the last and only setting in a section.

As discussed here, I think that auto-pruning empty sections is beyond the scope of the ini_setting type, and that removal should rather be explicitly requested with a dedicated ini_section type.

My proposal would be to remove the auto-removing of empty sections from ini_setting and introduce a new ini_section type that can be used to add / remove sections regardless of their content:

ini_section { 'section1':
  ensure => absent,
}

@peytonw1 would this allow you to do what you want?

[peytonw1]

@smortex yes that would, thanks!

[smortex]

@peytonw1 cool! There was some discussion here in case you missed it, with links to PR that fix the issue:
#532 (comment)