From ad5d92461596d8d0b1c001d49061d70b36761d59 Mon Sep 17 00:00:00 2001 From: Alex Harvey Date: Fri, 29 Sep 2017 16:14:37 +1000 Subject: (maint) Clarify docs and add new tests Based on a Stack Overflow question, it was noted that the documentation for `file_line` isn't complete and the underlying behaviour somewhat confusing. https://stackoverflow.com/questions/46468922/how-to-change-the-contents-of-a-file-by-using-puppet/46473458 In this patch I add additional unit tests and better examples and complete documentation. --- README.md | 46 +++++++++--- lib/puppet/type/file_line.rb | 24 ++++++- spec/unit/puppet/provider/file_line/ruby_spec.rb | 91 +++++++++++++++++++++++- spec/unit/puppet/type/file_line_spec.rb | 3 + 4 files changed, 150 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index ac4f997..2b6a64b 100644 --- a/README.md +++ b/README.md @@ -117,29 +117,55 @@ In the example above, `match` looks for a line beginning with 'export' followed Match Example: - file_line { 'bashrc_proxy': - ensure => present, - path => '/etc/bashrc', - line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', - match => '^export\ HTTP_PROXY\=', - append_on_no_match => false, - } +```puppet +file_line { 'bashrc_proxy': + ensure => present, + path => '/etc/bashrc', + line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', + match => '^export\ HTTP_PROXY\=', + append_on_no_match => false, +} +``` In this code example, `match` looks for a line beginning with export followed by HTTP_PROXY and replaces it with the value in line. If a match is not found, then no changes are made to the file. -Match Example with `ensure => absent`: +Examples With `ensure => absent`: + +This type has two behaviors when `ensure => absent` is set. + +One possibility is to set `match => ...` and `match_for_absence => true`, +as in the following example: ```puppet file_line { 'bashrc_proxy': ensure => absent, path => '/etc/bashrc', - line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', match => '^export\ HTTP_PROXY\=', match_for_absence => true, } ``` -In the example above, `match` looks for a line beginning with 'export' followed by 'HTTP_PROXY' and deletes it. If multiple lines match, an error is raised, unless the `multiple => true` parameter is set. +In this code example match will look for a line beginning with export +followed by HTTP_PROXY and delete it. If multiple lines match, an +error will be raised unless the `multiple => true` parameter is set. + +Note that the `line => ...` parameter would be accepted *but ignored* in +the above example. + +The second way of using `ensure => absent` is to specify a `line => ...`, +and no match: + +```puppet +file_line { 'bashrc_proxy': + ensure => absent, + path => '/etc/bashrc', + line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', +} +``` + +Note that when ensuring lines are absent this way, the default behavior +this time is to always remove all lines matching, and this behavior +can't be disabled. Encoding example: diff --git a/lib/puppet/type/file_line.rb b/lib/puppet/type/file_line.rb index b2357b8..06be552 100644 --- a/lib/puppet/type/file_line.rb +++ b/lib/puppet/type/file_line.rb @@ -34,12 +34,16 @@ Puppet::Type.newtype(:file_line) do In this code example match will look for a line beginning with export followed by HTTP_PROXY and replace it with the value in line. - Match Example With `ensure => absent`: + Examples With `ensure => absent`: + + This type has two behaviors when `ensure => absent` is set. + + One possibility is to set `match => ...` and `match_for_absence => true`, + as in the following example: file_line { 'bashrc_proxy': ensure => absent, path => '/etc/bashrc', - line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', match => '^export\ HTTP_PROXY\=', match_for_absence => true, } @@ -48,6 +52,22 @@ Puppet::Type.newtype(:file_line) do followed by HTTP_PROXY and delete it. If multiple lines match, an error will be raised unless the `multiple => true` parameter is set. + Note that the `line => ...` parameter would be accepted BUT IGNORED in + the above example. + + The second way of using `ensure => absent` is to specify a `line => ...`, + and no match: + + file_line { 'bashrc_proxy': + ensure => absent, + path => '/etc/bashrc', + line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', + } + + Note that when ensuring lines are absent this way, the default behavior + this time is to always remove all lines matching, and this behavior + can't be disabled. + Encoding example: file_line { "XScreenSaver": diff --git a/spec/unit/puppet/provider/file_line/ruby_spec.rb b/spec/unit/puppet/provider/file_line/ruby_spec.rb index dcca4a4..4ec9bae 100755 --- a/spec/unit/puppet/provider/file_line/ruby_spec.rb +++ b/spec/unit/puppet/provider/file_line/ruby_spec.rb @@ -337,7 +337,7 @@ describe provider_class, :unless => Puppet::Util::Platform.windows? do end end - context "when removing" do + context "when removing with a line" do before :each do # TODO: these should be ported over to use the PuppetLabs spec_helper # file fixtures once the following pull request has been merged: @@ -378,6 +378,23 @@ describe provider_class, :unless => Puppet::Util::Platform.windows? do @provider.destroy expect(File.read(@tmpfile)).to eql("foo1\nfoo2\n") end + + it 'example in the docs' do + @resource = Puppet::Type::File_line.new( + { + :name => 'bashrc_proxy', + :ensure => 'absent', + :path => @tmpfile, + :line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', + } + ) + @provider = provider_class.new(@resource) + File.open(@tmpfile, 'w') do |fh| + fh.write("foo1\nfoo2\nexport HTTP_PROXY=http://squid.puppetlabs.vm:3128\nfoo4\n") + end + @provider.destroy + expect(File.read(@tmpfile)).to eql("foo1\nfoo2\nfoo4\n") + end end context "when removing with a match" do @@ -416,6 +433,41 @@ describe provider_class, :unless => Puppet::Util::Platform.windows? do expect(File.read(@tmpfile)).to eql("foo1\nfoo2") end + it 'the line parameter is actually not used at all but is silently ignored if here' do + @resource = Puppet::Type::File_line.new( + { + :name => 'foo', + :path => @tmpfile, + :line => 'supercalifragilisticexpialidocious', + :ensure => 'absent', + :match => 'o$', + :match_for_absence => true, + } + ) + File.open(@tmpfile, 'w') do |fh| + fh.write("foo1\nfoo\nfoo2") + end + @provider.destroy + expect(File.read(@tmpfile)).to eql("foo1\nfoo2") + end + + it 'and may not be here and does not need to be here' do + @resource = Puppet::Type::File_line.new( + { + :name => 'foo', + :path => @tmpfile, + :ensure => 'absent', + :match => 'o$', + :match_for_absence => true, + } + ) + File.open(@tmpfile, 'w') do |fh| + fh.write("foo1\nfoo\nfoo2") + end + @provider.destroy + expect(File.read(@tmpfile)).to eql("foo1\nfoo2") + end + it 'should raise an error if more than one line matches' do File.open(@tmpfile, 'w') do |fh| fh.write("foo1\nfoo\nfoo2\nfoo\nfoo") @@ -480,6 +532,41 @@ describe provider_class, :unless => Puppet::Util::Platform.windows? do expect(File.read(@tmpfile)).to eql("foo1\nfoo\n") end - end + it 'example in the docs' do + @resource = Puppet::Type::File_line.new( + { + :name => 'bashrc_proxy', + :ensure => 'absent', + :path => @tmpfile, + :line => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128', + :match => '^export\ HTTP_PROXY\=', + :match_for_absence => true, + } + ) + @provider = provider_class.new(@resource) + File.open(@tmpfile, 'w') do |fh| + fh.write("foo1\nfoo2\nexport HTTP_PROXY=foo\nfoo4\n") + end + @provider.destroy + expect(File.read(@tmpfile)).to eql("foo1\nfoo2\nfoo4\n") + end + it 'example in the docs showing line is redundant' do + @resource = Puppet::Type::File_line.new( + { + :name => 'bashrc_proxy', + :ensure => 'absent', + :path => @tmpfile, + :match => '^export\ HTTP_PROXY\=', + :match_for_absence => true, + } + ) + @provider = provider_class.new(@resource) + File.open(@tmpfile, 'w') do |fh| + fh.write("foo1\nfoo2\nexport HTTP_PROXY=foo\nfoo4\n") + end + @provider.destroy + expect(File.read(@tmpfile)).to eql("foo1\nfoo2\nfoo4\n") + end + end end diff --git a/spec/unit/puppet/type/file_line_spec.rb b/spec/unit/puppet/type/file_line_spec.rb index 150149b..db07352 100755 --- a/spec/unit/puppet/type/file_line_spec.rb +++ b/spec/unit/puppet/type/file_line_spec.rb @@ -78,6 +78,9 @@ describe Puppet::Type.type(:file_line) do it 'should not require that a line is specified when matching for absence' do expect { Puppet::Type.type(:file_line).new(:name => 'foo', :path => tmp_path, :ensure => :absent, :match_for_absence => :true, :match => 'match') }.not_to raise_error end + it 'although if a line is specified anyway when matching for absence it still works and the line is silently ignored' do + expect { Puppet::Type.type(:file_line).new(:name => 'foo', :path => tmp_path, :line => 'i_am_irrelevant', :ensure => :absent, :match_for_absence => :true, :match => 'match') }.not_to raise_error + end it 'should require that a file is specified' do expect { Puppet::Type.type(:file_line).new(:name => 'foo', :line => 'path') }.to raise_error(Puppet::Error, /path is a required attribute/) end -- cgit v1.2.3