From 53a4c411952b140a7337114630bfce5f384369a7 Mon Sep 17 00:00:00 2001 From: Hugo Lopes Tavares Date: Tue, 15 Oct 2013 17:01:58 -0400 Subject: [PATCH] Bugfix for off-by-1 error in ResourceCollection#delete This fix may be temporary, since a new idea for how to implement ResourceCollection#delete popped up. Andrew Gross (@awgross) helped me find, debug, and write this patch, and he has a nice idea of how `unwind` should be implemented. His idea is that we should not remove the resource from ResourceCollection, because it may have unknown side effects (as it had and thus the bugfix), and instead we should replace the resource with a fake resource, a no operation (noop) valid resource, that when triggered, does nothing. We need to investigate that idea, but it sound great and more error-prone. --- lib/chef/rewind.rb | 6 ++++ spec/support/lib/chef/provider/cat.rb | 21 +++++++++++++ spec/support/lib/chef/provider/zen_master.rb | 13 ++++++++ spec/support/lib/chef/resource/cat.rb | 6 ++-- spec/support/lib/chef/resource/zen_master.rb | 6 ++++ spec/unwind_recipe_spec.rb | 33 ++++++++++++++++++-- 6 files changed, 78 insertions(+), 7 deletions(-) create mode 100644 spec/support/lib/chef/provider/cat.rb create mode 100644 spec/support/lib/chef/provider/zen_master.rb diff --git a/lib/chef/rewind.rb b/lib/chef/rewind.rb index b1aab52..38c2057 100644 --- a/lib/chef/rewind.rb +++ b/lib/chef/rewind.rb @@ -72,7 +72,13 @@ def delete_resource(resource_id) # assumes `resource_id` is the same as `Chef::Resource#to_s` @resources.delete_if {|r| r.to_s == resource_id } + resource_index_value = @resources_by_name[resource_id] + @resources_by_name.each do |k, v| + @resources_by_name[k] = v - 1 if v > resource_index_value + end + @resources_by_name.delete resource_id + end end end diff --git a/spec/support/lib/chef/provider/cat.rb b/spec/support/lib/chef/provider/cat.rb new file mode 100644 index 0000000..c853c1d --- /dev/null +++ b/spec/support/lib/chef/provider/cat.rb @@ -0,0 +1,21 @@ +class Chef + class Provider + class Cat < Chef::Provider + class CatError < RuntimeError + end + + def load_current_resource + true + end + + def action_sell + true + end + + def action_blowup + raise CatError, "CAT BLOWUP" + end + + end + end +end diff --git a/spec/support/lib/chef/provider/zen_master.rb b/spec/support/lib/chef/provider/zen_master.rb new file mode 100644 index 0000000..41b570e --- /dev/null +++ b/spec/support/lib/chef/provider/zen_master.rb @@ -0,0 +1,13 @@ +class Chef + class Provider + class ZenMaster < Chef::Provider + def load_current_resource + true + end + + def action_change + true + end + end + end +end diff --git a/spec/support/lib/chef/resource/cat.rb b/spec/support/lib/chef/resource/cat.rb index 5809ad3..85e1379 100644 --- a/spec/support/lib/chef/resource/cat.rb +++ b/spec/support/lib/chef/resource/cat.rb @@ -19,13 +19,11 @@ class Chef class Resource class Cat < Chef::Resource - - attr_accessor :action - def initialize(name, run_context=nil) @resource_name = :cat super - @action = "sell" + @action = :nothing + @allowed_actions = [:nothing, :sell, :blowup] end def pretty_kitty(arg=nil) diff --git a/spec/support/lib/chef/resource/zen_master.rb b/spec/support/lib/chef/resource/zen_master.rb index 71842b7..5c4fac7 100644 --- a/spec/support/lib/chef/resource/zen_master.rb +++ b/spec/support/lib/chef/resource/zen_master.rb @@ -27,12 +27,18 @@ class ZenMaster < Chef::Resource def initialize(name, run_context=nil) @resource_name = :zen_master super + @action = :nothing + @allowed_actions = [:change, :nothing] end def peace(tf) @peace = tf end + def updated_by_last_action? + true + end + def something(arg=nil) set_if_args(@something, arg) do case arg diff --git a/spec/unwind_recipe_spec.rb b/spec/unwind_recipe_spec.rb index 99c2acc..b0b5a72 100644 --- a/spec/unwind_recipe_spec.rb +++ b/spec/unwind_recipe_spec.rb @@ -2,17 +2,21 @@ require 'chef/rewind' describe Chef::Recipe do - + before(:each) do @cookbook_repo = File.expand_path(File.join(File.dirname(__FILE__), "..", "data", "cookbooks")) cl = Chef::CookbookLoader.new(@cookbook_repo) cl.load_cookbooks @cookbook_collection = Chef::CookbookCollection.new(cl) @node = Chef::Node.new + @node.name "latte" + @node.automatic[:platform] = "mac_os_x" + @node.automatic[:platform_version] = "10.5.1" @node.normal[:tags] = Array.new @events = Chef::EventDispatch::Dispatcher.new @run_context = Chef::RunContext.new(@node, @cookbook_collection, @events) @recipe = Chef::Recipe.new("hjk", "test", @run_context) + @runner = Chef::Runner.new(@run_context) end @@ -21,15 +25,38 @@ @recipe.zen_master "foobar" do peace false end - + @recipe.unwind "zen_master[foobar]" resources = @run_context.resource_collection.all_resources resources.length.should == 0 end + it "should define resource completely when unwind is called" do + @recipe.zen_master "foo" do + action :nothing + peace false + end + @recipe.cat "blanket" do + end + @recipe.zen_master "bar" do + action :nothing + peace false + end + + @recipe.unwind "zen_master[foo]" + + @recipe.zen_master "foobar" do + peace true + action :change + notifies :blowup, "cat[blanket]" + end + + lambda { @runner.converge }.should raise_error(Chef::Provider::Cat::CatError) + end + it "should throw an error when unwinding a nonexistent resource" do - lambda do + lambda do @recipe.unwind "zen_master[foobar]" end.should raise_error(Chef::Exceptions::ResourceNotFound) end