Hi Mike,
Thank you for submitting these changes. I made a few slight changes --
removed an extra call to aug.match and called resource.noop? instead
of Puppet[:noop]. I also modified the spec tests you added to not stub
the augeas resource. We can modify the other spec tests when we tackle
http://projects.puppetlabs.com/issues/7592.
Thanks,
Josh
diff --git a/lib/puppet/provider/augeas/augeas.rb b/lib/puppet/
provider/augeas/augeas.rb
index 643462e..0ebf197 100644
--- a/lib/puppet/provider/augeas/augeas.rb
+++ b/lib/puppet/provider/augeas/augeas.rb
@@ -259,11 +259,6 @@ Puppet::Type.type(:augeas).provide(:augeas) do
@aug.set("/augeas/save", mode)
end
- def files_changed?
- saved_files = @aug.match("/augeas/events/saved")
- saved_files.size > 0
- end
-
# Determines if augeas acutally needs to run.
def need_to_run?
force = resource[:force]
@@ -296,14 +291,14 @@ Puppet::Type.type(:augeas).provide(:augeas) do
do_execute_changes
save_result = @aug.save
saved_files = @aug.match("/augeas/events/saved")
- if save_result and files_changed?
+ if save_result and saved_files.size > 0
root = resource[:root].sub(/^\/$/, "")
saved_files.each do |key|
saved_file = @aug.get(key).to_s.sub(/^\/files/, root)
if Puppet[:show_diff]
print diff(saved_file, saved_file + ".augnew")
end
- if Puppet[:noop]
+ if resource.noop?
File.delete(saved_file + ".augnew")
end
end
@@ -316,7 +311,7 @@ Puppet::Type.type(:augeas).provide(:augeas) do
end
end
ensure
- if not return_value or Puppet[:noop]
+ if not return_value or resource.noop?
close_augeas
end
end
diff --git a/spec/unit/provider/augeas/augeas_spec.rb b/spec/unit/
provider/augeas/augeas_spec.rb
index aeeb196..687043b 100755
--- a/spec/unit/provider/augeas/augeas_spec.rb
+++ b/spec/unit/provider/augeas/augeas_spec.rb
@@ -4,7 +4,6 @@ require 'spec_helper'
provider_class = Puppet::Type.type(:augeas).provider(:augeas)
describe provider_class do
-
describe "command parsing" do
before do
@resource = stub("resource")
@@ -249,13 +248,10 @@ describe provider_class do
end
describe "need to run" do
- before do
- File.stubs(:delete)
- end
-
it "should handle no filters" do
resource = stub("resource")
resource.stubs(:
[]).returns(false).then.returns("").then.returns("")
+ resource.stubs(:noop?).returns(false)
augeas_stub = stub("augeas", :match => ["set", "of", "values"])
augeas_stub.stubs("close")
provider = provider_class.new(resource)
@@ -267,6 +263,7 @@ describe provider_class do
it "should return true when a get filter matches" do
resource = stub("resource")
resource.stubs(:[]).returns(false).then.returns("get path ==
value").then.returns("")
+ resource.stubs(:noop?).returns(false)
provider = provider_class.new(resource)
augeas_stub = stub("augeas", :get => "value")
augeas_stub.stubs("close")
@@ -289,6 +286,7 @@ describe provider_class do
it "should return true when a match filter matches" do
resource = stub("resource")
resource.stubs(:[]).returns(false).then.returns("match path
size == 3").then.returns("")
+ resource.stubs(:noop?).returns(false)
provider = provider_class.new(resource)
augeas_stub = stub("augeas", :match => ["set", "of", "values"])
augeas_stub.stubs("close")
@@ -324,6 +322,7 @@ describe provider_class do
it "should return true when a size != the provided value" do
resource = stub("resource")
resource.stubs(:[]).returns(false).then.returns("match path
size != 17").then.returns("")
+ resource.stubs(:noop?).returns(false)
provider = provider_class.new(resource)
augeas_stub = stub("augeas", :match => ["set", "of", "values"])
augeas_stub.stubs("close")
@@ -348,92 +347,101 @@ describe provider_class do
describe "and Puppet[:show_diff] is set" do
before do
Puppet[:show_diff] = true
+
+ @resource = Puppet::Type.type(:augeas).new(:name => "test")
+ @provider = provider_class.new(@resource)
+ @augeas_stub = stub("augeas")
+ @provider.aug = @augeas_stub
+
+ @augeas_stub.stubs("get").with("/augeas/
version").returns("0.7.2")
+ @augeas_stub.stubs(:set).returns(true)
+ @augeas_stub.stubs(:save).returns(true)
end
it "should call diff when a file is shown to have been changed"
do
file = "/etc/hosts"
- resource = stub("resource")
- resource.stubs(:[]).returns(false).then.returns("set /files/
foo bar").then.returns("")
- provider = provider_class.new(resource)
- augeas_stub = stub("augeas")
- augeas_stub.expects("set").with("/augeas/save", "newfile")
- augeas_stub.expects("save").returns(true)
- augeas_stub.expects("get").with("/augeas/events/
saved").returns(["/files#{file}"])
- augeas_stub.stubs("match").with("/augeas/events/
saved").returns(["/augeas/events/saved"])
- augeas_stub.stubs("close")
-
- provider.aug= augeas_stub
- provider.stubs(:get_augeas_version).returns("0.7.2")
- provider.expects(:diff).with("#{file}", "#{file}.augnew")
- provider.need_to_run?.should == true
+ @resource[:context] = "/files"
+ @resource[:changes] = ["set #{file}/foo bar"]
+
+ @augeas_stub.stubs(:match).with("/augeas/events/
saved").returns(["/augeas/events/saved"])
+ @augeas_stub.stubs(:get).with("/augeas/events/
saved").returns(["/files#{file}"])
+ @augeas_stub.expects(:set).with("/augeas/save", "newfile")
+ @augeas_stub.expects(:close).never()
+
+ @provider.expects("diff").with("#{file}",
"#{file}.augnew").returns("")
+ @provider.should be_need_to_run
end
it "should call diff for each file thats changed" do
file1 = "/etc/hosts"
file2 = "/etc/resolv.conf"
- resource = stub("resource")
- resource.stubs(:[]).returns(false).then.returns(["set /files/
foo bar", "set /files/bar foo"]).then.returns("")
- provider = provider_class.new(resource)
- augeas_stub = stub("augeas")
- augeas_stub.expects("set").with("/augeas/save", "newfile")
- augeas_stub.expects("save").returns(true)
- augeas_stub.expects("get").with("/augeas/events/
saved[1]").returns(["/files#{file1}"])
- augeas_stub.expects("get").with("/augeas/events/
saved[2]").returns(["/files#{file2}"])
- augeas_stub.stubs("match").with("/augeas/events/
saved").returns(["/augeas/events/saved[1]", "/augeas/events/
saved[2]"])
- augeas_stub.stubs("close")
-
- provider.aug= augeas_stub
- provider.stubs(:get_augeas_version).returns("0.7.2")
- provider.expects(:diff).with("#{file1}", "#{file1}.augnew")
- provider.expects(:diff).with("#{file2}", "#{file2}.augnew")
- provider.need_to_run?.should == true
+ @resource[:context] = "/files"
+ @resource[:changes] = ["set #{file1}/foo bar", "set #{file2}/
baz biz"]
+
+ @augeas_stub.stubs(:match).with("/augeas/events/
saved").returns(["/augeas/events/saved[1]", "/augeas/events/
saved[2]"])
+ @augeas_stub.stubs(:get).with("/augeas/events/
saved[1]").returns(["/files#{file1}"])
+ @augeas_stub.stubs(:get).with("/augeas/events/
saved[2]").returns(["/files#{file2}"])
+ @augeas_stub.expects(:set).with("/augeas/save", "newfile")
+ @augeas_stub.expects(:close).never()
+
+ @provider.expects(:diff).with("#{file1}",
"#{file1}.augnew").returns("")
+ @provider.expects(:diff).with("#{file2}",
"#{file2}.augnew").returns("")
+ @provider.should be_need_to_run
end
describe "and resource[:root] is set" do
- before do
- end
-
it "should call diff when a file is shown to have been
changed" do
root = "/tmp/foo"
file = "/etc/hosts"
- resource = stub("resource")
- resource.stubs(:root).returns("#{root}")
- resource.stubs(:[]).returns(false).then.returns("set /files/
foo bar").then.returns("")
- provider = provider_class.new(resource)
- augeas_stub = stub("augeas")
- augeas_stub.expects("set").with("/augeas/save", "newfile")
- augeas_stub.expects("save").returns(true)
- augeas_stub.expects("get").with("/augeas/events/
saved").returns(["/files#{root}#{file}"])
- augeas_stub.stubs("match").with("/augeas/events/
saved").returns(["/augeas/events/saved"])
- augeas_stub.stubs("close")
-
- provider.aug= augeas_stub
- provider.stubs(:get_augeas_version).returns("0.7.2")
- provider.expects(:diff).with("#{root}#{file}", "#{root}
#{file}.augnew")
- provider.need_to_run?.should == true
+ @resource[:context] = "/files"
+ @resource[:changes] = ["set #{file}/foo bar"]
+ @resource[:root] = root
+
+ @augeas_stub.stubs(:match).with("/augeas/events/
saved").returns(["/augeas/events/saved"])
+ @augeas_stub.stubs(:get).with("/augeas/events/
saved").returns(["/files#{file}"])
+ @augeas_stub.expects(:set).with("/augeas/save", "newfile")
+ @augeas_stub.expects(:close).never()
+
+ @provider.expects(:diff).with("#{root}#{file}", "#{root}
#{file}.augnew").returns("")
+ @provider.should be_need_to_run
end
end
- end
- it "should not call diff if no files change" do
- file = "/etc/hosts"
+ it "should not call diff if no files change" do
+ file = "/etc/hosts"
- resource = stub("resource")
- resource.stubs(:[]).returns(false).then.returns("set /files/foo
bar").then.returns("")
- provider = provider_class.new(resource)
- augeas_stub = stub("augeas")
- augeas_stub.expects("set").with("/augeas/save", "newfile")
- augeas_stub.expects("save").returns(true)
- augeas_stub.stubs("match").with("/augeas/events/
saved").returns([])
- augeas_stub.stubs("close")
+ @resource[:context] = "/files"
+ @resource[:changes] = ["set #{file}/foo bar"]
- provider.aug= augeas_stub
- provider.stubs(:get_augeas_version).returns("0.7.2")
- provider.expects(:diff).never
- provider.need_to_run?.should == false
+ @augeas_stub.stubs(:match).with("/augeas/events/
saved").returns([])
+ @augeas_stub.expects(:set).with("/augeas/save", "newfile")
+ @augeas_stub.expects(:get).with("/augeas/events/
saved").never()
+ @augeas_stub.expects(:close)
+
+ @provider.expects(:diff).never()
+ @provider.should_not be_need_to_run
+ end
+
+ it "should cleanup when in noop mode" do
+ file = "/etc/hosts"
+
+ @resource[:noop] = true
+ @resource[:context] = "/files"
+ @resource[:changes] = ["set #{file}/foo bar"]
+
+ @augeas_stub.stubs(:match).with("/augeas/events/
saved").returns(["/augeas/events/saved"])
+ @augeas_stub.stubs(:get).with("/augeas/events/
saved").returns(["/files#{file}"])
+ @augeas_stub.expects(:set).with("/augeas/save", "newfile")
+ @augeas_stub.expects(:close)
+
+ File.expects(:delete).with(file + ".augnew")
+
+ @provider.expects(:diff).with("#{file}",
"#{file}.augnew").returns("")
+ @provider.should be_need_to_run
+ end
end
end
On May 29, 10:02 pm, Michael Knox <michael.knox...@gmail.com> wrote:
> From: Michael Knox <m...@sysam.com.au>
>
> Utilising Augeas's SAVE_NEWFILE mode (similar to augtool -n) to
> determine the changes that will be made be made by Augeas.
> Output a unified diff to info
>
> handle non-default root, and multiple files correctly
>
> Adding tests for Augeas diff functionality
> Add test for non-default :root when diff'ing
> Ensure that multiple files are diffed if changed, not just one
>
> Signed-off-by: Mike Knox <m...@hfnix.net>
> ---
> Local-branch: feature/master/2728
> lib/puppet/provider/augeas/augeas.rb | 54 +++++++++++++----
> spec/unit/provider/augeas/augeas_spec.rb | 97
> ++++++++++++++++++++++++++++++
> 2 files changed, 139 insertions(+), 12 deletions(-)
>
> diff --git a/lib/puppet/provider/augeas/augeas.rb
> b/lib/puppet/provider/augeas/augeas.rb
> index a16f54b..643462e 100644
> --- a/lib/puppet/provider/augeas/augeas.rb
> +++ b/lib/puppet/provider/augeas/augeas.rb
> @@ -15,9 +15,12 @@
>
> require 'augeas' if Puppet.features.augeas?
> require 'strscan'
> +require 'puppet/util'
> +require 'puppet/util/diff'
>
> Puppet::Type.type(:augeas).provide(:augeas) do
> include Puppet::Util
> + include Puppet::Util::Diff
>
> confine :true => Puppet.features.augeas?
>
> @@ -25,6 +28,8 @@ Puppet::Type.type(:augeas).provide(:augeas) do
>
> SAVE_NOOP = "noop"
> SAVE_OVERWRITE = "overwrite"
> + SAVE_NEWFILE = "newfile"
> + SAVE_BACKUP = "backup"
>
> COMMANDS = {
> "set" => [ :path, :string ],
> @@ -287,20 +292,33 @@ Puppet::Type.type(:augeas).provide(:augeas) do
> # actually do the save.
> if return_value and get_augeas_version >= "0.3.6"
> debug("Will attempt to save and only run if files changed")
> - set_augeas_save_mode(SAVE_NOOP)
> + set_augeas_save_mode(SAVE_NEWFILE)
> do_execute_changes
> save_result = @aug.save
> saved_files = @aug.match("/augeas/events/saved")
> - if save_result and not files_changed?
> - debug("Skipping because no files were changed")
> - return_value = false
> - else
> + if save_result and files_changed?
> + root = resource[:root].sub(/^\/$/, "")
> + saved_files.each do |key|
> + saved_file = @aug.get(key).to_s.sub(/^\/files/, root)
> + if Puppet[:show_diff]
> + print diff(saved_file, saved_file + ".augnew")
> + end
> + if Puppet[:noop]
> + File.delete(saved_file + ".augnew")
> + end
> + end
> debug("Files changed, should execute")
> + return_value = true
> + else
> + debug("Skipping because no files were changed or save failed")
> + return_value = false
> end
> end
> end
> ensure
> - close_augeas
> + if not return_value or Puppet[:noop]
> + close_augeas
> + end
> end
> return_value
> end
> @@ -309,12 +327,24 @@ Puppet::Type.type(:augeas).provide(:augeas) do
> # Re-connect to augeas, and re-execute the changes
> begin
> open_augeas
> - set_augeas_save_mode(SAVE_OVERWRITE) if get_augeas_version >= "0.3.6"
> -
> - do_execute_changes
> -
> - success = @aug.save
> - fail("Save failed with return code #{success}") if success != true
> + saved_files = @aug.match("/augeas/events/saved")
> + if saved_files
> + saved_files.each do |key|
> + root = resource[:root].sub(/^\/$/, "")
> + saved_file = @aug.get(key).to_s.sub(/^\/files/, root)
> + if File.exists?(saved_file + ".augnew")
> + success = File.rename(saved_file + ".augnew", saved_file)
> + debug(saved_file + ".augnew moved to " + saved_file)
> + fail("Rename failed with return code #{success}") if success != 0
> + end
> + end
> + else
> + debug("No saved files, re-executing augeas")
> + set_augeas_save_mode(SAVE_OVERWRITE) if get_augeas_version >= "0.3.6"
> + do_execute_changes
> + success = @aug.save
> + fail("Save failed with return code #{success}") if success != true
> + end
> ensure
> close_augeas
> end
> diff --git a/spec/unit/provider/augeas/augeas_spec.rb
> b/spec/unit/provider/augeas/augeas_spec.rb
> index 5bb98ea..aeeb196 100755
> --- a/spec/unit/provider/augeas/augeas_spec.rb
> +++ b/spec/unit/provider/augeas/augeas_spec.rb
> @@ -249,6 +249,10 @@ describe provider_class do
> end
>
> describe "need to run" do
> + before do
> + File.stubs(:delete)
> + end
> +
> it "should handle no filters" do
> resource = stub("resource")
> resource.stubs(:[]).returns(false).then.returns("").then.returns("")
> @@ -339,6 +343,98 @@ describe provider_class do
> provider.stubs(:get_augeas_version).returns("0.3.5")
> provider.need_to_run?.should == false
> end
> +
> + # Ticket2728(diff files)
> + describe "and Puppet[:show_diff] is set" do
> + before do
> + Puppet[:show_diff] = true
> + end
> +
> + it "should call diff when a file is shown to have been changed" do
> + file = "/etc/hosts"
> +
> + resource = stub("resource")
> + resource.stubs(:[]).returns(false).then.returns("set /files/foo
> bar").then.returns("")
> + provider = provider_class.new(resource)
> + augeas_stub = stub("augeas")
> + augeas_stub.expects("set").with("/augeas/save", "newfile")
> + augeas_stub.expects("save").returns(true)
> +
> augeas_stub.expects("get").with("/augeas/events/saved").returns(["/files#{f
> ile}"])
> +
> augeas_stub.stubs("match").with("/augeas/events/saved").returns(["/augeas/e
> vents/saved"])
> + augeas_stub.stubs("close")
> +
> + provider.aug= augeas_stub
> + provider.stubs(:get_augeas_version).returns("0.7.2")
> + provider.expects(:diff).with("#{file}", "#{file}.augnew")
> + provider.need_to_run?.should == true
> + end
> +
> + it "should call diff for each file thats changed" do
> + file1 = "/etc/hosts"
> + file2 = "/etc/resolv.conf"
> +
> + resource = stub("resource")
> + resource.stubs(:[]).returns(false).then.returns(["set /files/foo
> bar", "set /files/bar foo"]).then.returns("")
> + provider = provider_class.new(resource)
> + augeas_stub = stub("augeas")
> + augeas_stub.expects("set").with("/augeas/save", "newfile")
> + augeas_stub.expects("save").returns(true)
> +
> augeas_stub.expects("get").with("/augeas/events/saved[1]").returns(["/files
> #{file1}"])
> +
> augeas_stub.expects("get").with("/augeas/events/saved[2]").returns(["/files
> #{file2}"])
> +
> augeas_stub.stubs("match").with("/augeas/events/saved").returns(["/augeas/e
> vents/saved[1]", "/augeas/events/saved[2]"])
> + augeas_stub.stubs("close")
> +
> + provider.aug= augeas_stub
> + provider.stubs(:get_augeas_version).returns("0.7.2")
> + provider.expects(:diff).with("#{file1}", "#{file1}.augnew")
> + provider.expects(:diff).with("#{file2}", "#{file2}.augnew")
> + provider.need_to_run?.should == true
> + end
> +
> + describe "and resource[:root] is set" do
> + before do
> + end
> +
> + it "should call diff when a file is shown to have been changed" do
> + root = "/tmp/foo"
> + file = "/etc/hosts"
> +
> + resource = stub("resource")
> + resource.stubs(:root).returns("#{root}")
> + resource.stubs(:[]).returns(false).then.returns("set /files/foo
> bar").then.returns("")
> + provider = provider_class.new(resource)
> + augeas_stub = stub("augeas")
> + augeas_stub.expects("set").with("/augeas/save", "newfile")
> + augeas_stub.expects("save").returns(true)
> +
> augeas_stub.expects("get").with("/augeas/events/saved").returns(["/files#{r
> oot}#{file}"])
> +
> augeas_stub.stubs("match").with("/augeas/events/saved").returns(["/augeas/e
> vents/saved"])
> + augeas_stub.stubs("close")
> +
> + provider.aug= augeas_stub
> + provider.stubs(:get_augeas_version).returns("0.7.2")
> + provider.expects(:diff).with("#{root}#{file}",
> "#{root}#{file}.augnew")
> + provider.need_to_run?.should == true
> + end
> + end
> + end
> +
> + it "should not call diff if no files change" do
> + file = "/etc/hosts"
> +
> + resource = stub("resource")
> + resource.stubs(:[]).returns(false).then.returns("set /files/foo
> bar").then.returns("")
> + provider = provider_class.new(resource)
> + augeas_stub = stub("augeas")
> + augeas_stub.expects("set").with("/augeas/save", "newfile")
> + augeas_stub.expects("save").returns(true)
> + augeas_stub.stubs("match").with("/augeas/events/saved").returns([])
> + augeas_stub.stubs("close")
> +
> + provider.aug= augeas_stub
> + provider.stubs(:get_augeas_version).returns("0.7.2")
> + provider.expects(:diff).never
> + provider.need_to_run?.should == false
> + end
> end
>
> describe "augeas execution integration" do
> @@ -349,6 +445,7 @@ describe provider_class do
> @augeas = stub("augeas")
> @provider.aug= @augeas
> @provider.stubs(:get_augeas_version).returns("0.3.5")
> + @augeas.stubs(:match).with("/augeas/events/saved")
> end
>
> it "should handle set commands" do
> --
> 1.7.3.4
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
