This is again about moving transactional behaviour out of Puppet::Type and into the transactional code.
I initially moved this code into Resource::Status, but it seemed to make more sense in the Harness - the Status object should be thin and have no code that doesn't make sense on both sides of the pipe, it seemed. The interface gets a bit uglier as a result, but this is all about good design != good OO (because we're increasing the argument count of methods by moving them outside of the target class). Signed-off-by: Luke Kanies <[email protected]> --- lib/puppet/transaction.rb | 2 +- lib/puppet/transaction/resource_harness.rb | 18 ++++++++++++++---- lib/puppet/type.rb | 12 ------------ spec/integration/transaction.rb | 2 +- spec/unit/transaction.rb | 2 +- spec/unit/transaction/resource_harness.rb | 27 +++++++++++++++++++++------ 6 files changed, 38 insertions(+), 25 deletions(-) diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb index d2dbf7a..8856324 100644 --- a/lib/puppet/transaction.rb +++ b/lib/puppet/transaction.rb @@ -318,7 +318,7 @@ class Puppet::Transaction # Is the resource currently scheduled? def scheduled?(resource) - self.ignoreschedules or resource_harness.scheduled?(resource) + self.ignoreschedules or resource_harness.scheduled?(resource_status(resource), resource) end # Should this resource be skipped? diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb index 05e569b..17e8dfa 100644 --- a/lib/puppet/transaction/resource_harness.rb +++ b/lib/puppet/transaction/resource_harness.rb @@ -23,10 +23,20 @@ class Puppet::Transaction::ResourceHarness status.changed = true end + # Used mostly for scheduling at this point. + def cached(resource, name) + Puppet::Util::Storage.cache(resource)[name] + end + + # Used mostly for scheduling at this point. + def cache(resource, name, value) + Puppet::Util::Storage.cache(resource)[name] = value + end + def changes_to_perform(status, resource) current = resource.retrieve - resource.cache :checked, Time.now + cache resource, :checked, Time.now return [] if ! allow_changes?(resource) @@ -54,7 +64,7 @@ class Puppet::Transaction::ResourceHarness status.change_count = changes.length apply_changes(status, changes) if ! resource.noop? - resource.cache(:synced, Time.now) + cache(resource, :synced, Time.now) resource.flush if resource.respond_to?(:flush) end end @@ -73,7 +83,7 @@ class Puppet::Transaction::ResourceHarness @transaction = transaction end - def scheduled?(resource) + def scheduled?(status, resource) return true if Puppet[:ignoreschedules] return true unless schedule = schedule(resource) @@ -82,7 +92,7 @@ class Puppet::Transaction::ResourceHarness # have been synced a long time ago (e.g., a file only gets updated # once a month on the server and its schedule is daily; the last sync time # will have been a month ago, so we'd end up checking every run). - return schedule.match?(resource.cached(:checked).to_i) + return schedule.match?(cached(resource, :checked).to_i) end def schedule(resource) diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb index 4d7c0aa..d0ea80f 100644 --- a/lib/puppet/type.rb +++ b/lib/puppet/type.rb @@ -1910,18 +1910,6 @@ class Type end.flatten.reject { |r| r.nil? } end - # Return a cached value - def cached(name) - Puppet::Util::Storage.cache(self)[name] - #...@cache[name] ||= nil - end - - # Cache a value - def cache(name, value) - Puppet::Util::Storage.cache(self)[name] = value - #...@cache[name] = value - end - # For now, leave the 'name' method functioning like it used to. Once 'title' # works everywhere, I'll switch it. def name diff --git a/spec/integration/transaction.rb b/spec/integration/transaction.rb index f682874..6a4dbae 100755 --- a/spec/integration/transaction.rb +++ b/spec/integration/transaction.rb @@ -242,6 +242,6 @@ describe Puppet::Transaction do trans = catalog.apply - trans.resource_harness.should be_scheduled(resource) + trans.resource_harness.should be_scheduled(trans.resource_status(resource), resource) end end diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb index 0c3d901..c5b0ea4 100755 --- a/spec/unit/transaction.rb +++ b/spec/unit/transaction.rb @@ -316,7 +316,7 @@ describe Puppet::Transaction do end it "should let the resource harness determine whether the resource should be scheduled" do - @transaction.resource_harness.expects(:scheduled?).with(@resource).returns "feh" + @transaction.resource_harness.expects(:scheduled?).with(@transaction.resource_status(@resource), @resource).returns "feh" @transaction.scheduled?(@resource).should == "feh" end diff --git a/spec/unit/transaction/resource_harness.rb b/spec/unit/transaction/resource_harness.rb index 334dbff..ee2726d 100755 --- a/spec/unit/transaction/resource_harness.rb +++ b/spec/unit/transaction/resource_harness.rb @@ -62,7 +62,7 @@ describe Puppet::Transaction::ResourceHarness do changes = %w{mychanges} @harness.stubs(:changes_to_perform).returns changes @harness.stubs(:apply_changes) - @resource.expects(:cache).with { |name, time| name == :synced and time.is_a?(Time) } + @harness.expects(:cache).with { |resource, name, time| name == :synced and time.is_a?(Time) } @harness.evaluate(@resource) end @@ -110,7 +110,7 @@ describe Puppet::Transaction::ResourceHarness do end it "should cache that the resource was checked" do - @resource.expects(:cache).with { |name, time| name == :checked and time.is_a?(Time) } + @harness.expects(:cache).with { |resource, name, time| name == :checked and time.is_a?(Time) } @harness.changes_to_perform(@status, @resource) end @@ -301,21 +301,22 @@ describe Puppet::Transaction::ResourceHarness do before do @catalog = Puppet::Resource::Catalog.new @resource.catalog = @catalog + @status = Puppet::Resource::Status.new(@resource) end it "should return true if 'ignoreschedules' is set" do Puppet[:ignoreschedules] = true @resource[:schedule] = "meh" - @harness.should be_scheduled(@resource) + @harness.should be_scheduled(@status, @resource) end it "should return true if the resource has no schedule set" do - @harness.should be_scheduled(@resource) + @harness.should be_scheduled(@status, @resource) end it "should return the result of matching the schedule with the cached 'checked' time if a schedule is set" do t = Time.now - @resource.expects(:cached).with(:checked).returns(t) + @harness.expects(:cached).with(@resource, :checked).returns(t) sched = Puppet::Type.type(:schedule).new(:name => "sched") @catalog.add_resource(sched) @@ -323,7 +324,21 @@ describe Puppet::Transaction::ResourceHarness do sched.expects(:match?).with(t.to_i).returns "feh" - @harness.scheduled?(@resource).should == "feh" + @harness.scheduled?(@status, @resource).should == "feh" end end + + it "should be able to cache data in the Storage module" do + data = {} + Puppet::Util::Storage.expects(:cache).with(@resource).returns data + @harness.cache(@resource, :foo, "something") + + data[:foo].should == "something" + end + + it "should be able to retrieve data from the cache" do + data = {:foo => "other"} + Puppet::Util::Storage.expects(:cache).with(@resource).returns data + @harness.cached(@resource, :foo).should == "other" + end end -- 1.6.1 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
