From 039661156116cb6d3c5c8e372ed4d81af221453b Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Thu, 21 Jul 2011 15:35:35 -0700 Subject: maint: better error reporting when test fails This test ensures, among other things, that we get a log message. If that fails, we were trying to call a random method on nil; making that an assertion means that we get a nice message rather than a failure that needs decoding. Reviewed-By: Pieter van de Bruggen --- spec/unit/application/face_base_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/unit/application/face_base_spec.rb b/spec/unit/application/face_base_spec.rb index 0a4a86be6..bebc26210 100755 --- a/spec/unit/application/face_base_spec.rb +++ b/spec/unit/application/face_base_spec.rb @@ -55,6 +55,7 @@ describe Puppet::Application::FaceBase do it "should stop if the first thing found is not an action" do app.command_line.stubs(:args).returns %w{banana count_args} expect { app.run }.to exit_with 1 + @logs.first.should_not be_nil @logs.first.message.should =~ /has no 'banana' action/ end -- cgit From 1e0655e6bdbc872014abdffa5deacb334616e826 Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Thu, 21 Jul 2011 15:34:52 -0700 Subject: (#7184) Centralize "find action for face" into Puppet::Face As part of moving to load actions first, and their associated face, when invoked from the command line, it makes sense to push the logic for finding the action and face down into the Puppet::Face implementation. This means that we can change the logic there without needing to update the public part of the CLI implementation, and that any further facades can use the same, correct, logic to locate the action for the face. Reviewed-By: Pieter van de Bruggen --- lib/puppet/application/face_base.rb | 3 ++- lib/puppet/interface.rb | 4 ++++ lib/puppet/interface/action.rb | 1 + lib/puppet/interface/face_collection.rb | 11 +++++++++++ 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lib/puppet/application/face_base.rb b/lib/puppet/application/face_base.rb index ea5ba4aaf..a111518f1 100644 --- a/lib/puppet/application/face_base.rb +++ b/lib/puppet/application/face_base.rb @@ -100,7 +100,8 @@ class Puppet::Application::FaceBase < Puppet::Application # action object it represents; if this is an invalid action name that # will be nil, and handled later. action_name = item.to_sym - @action = @face.get_action(action_name) + @action = Puppet::Face.find_action(@face.name, action_name) + @face = @action.face if @action end end diff --git a/lib/puppet/interface.rb b/lib/puppet/interface.rb index 6c288f3c0..eba99d6be 100644 --- a/lib/puppet/interface.rb +++ b/lib/puppet/interface.rb @@ -64,6 +64,10 @@ class Puppet::Interface end face end + + def find_action(name, action, version = :current) + Puppet::Interface::FaceCollection.get_action_for_face(name, action, version) + end end def set_default_format(format) diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb index fc1121eb6..ce9c60b49 100644 --- a/lib/puppet/interface/action.rb +++ b/lib/puppet/interface/action.rb @@ -38,6 +38,7 @@ class Puppet::Interface::Action def to_s() "#{@face}##{@name}" end attr_reader :name + attr_reader :face attr_accessor :default def default? !!@default diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb index 4522824fd..ddc66f583 100644 --- a/lib/puppet/interface/face_collection.rb +++ b/lib/puppet/interface/face_collection.rb @@ -20,6 +20,17 @@ module Puppet::Interface::FaceCollection get_face(name, version) or load_face(name, version) end + def self.get_action_for_face(face_name, action_name, version) + # If the version they request specifically doesn't exist, don't search + # elsewhere. Usually this will start from :current and all... + return nil unless face = self[face_name, version] + unless action = face.get_action(action_name) + # ...we need to search for it bound to an o{lder,ther} version. + end + + return action + end + # get face from memory, without loading. def self.get_face(name, pattern) return nil unless @faces.has_key? name -- cgit From 2cd3bc47993fbd32a77ca9dfdd51353f2dfbcb58 Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Thu, 21 Jul 2011 16:34:20 -0700 Subject: (#7184) Find actions bound to other versions of Faces. When we first touch a Face, we load all the available Actions from disk. Given they define themselves against a specific version of a Face, they are automatically available tied to the correct version; this makes it trivially possible to locate those on demand and return them. Now, we have the ability to find and, consequently, invoke Actions on older versions of Faces. We don't load enough context, though: the older face will only have external Actions defined, not anything core. Reviewed-By: Pieter van de Bruggen --- lib/puppet/interface/face_collection.rb | 13 ++++++++++--- spec/lib/puppet/face/huzzah/obsolete.rb | 6 ++++++ spec/unit/interface/face_collection_spec.rb | 15 +++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 spec/lib/puppet/face/huzzah/obsolete.rb diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb index ddc66f583..868997b67 100644 --- a/lib/puppet/interface/face_collection.rb +++ b/lib/puppet/interface/face_collection.rb @@ -20,12 +20,19 @@ module Puppet::Interface::FaceCollection get_face(name, version) or load_face(name, version) end - def self.get_action_for_face(face_name, action_name, version) + def self.get_action_for_face(name, action_name, version) + name = underscorize(name) + # If the version they request specifically doesn't exist, don't search # elsewhere. Usually this will start from :current and all... - return nil unless face = self[face_name, version] + return nil unless face = self[name, version] unless action = face.get_action(action_name) - # ...we need to search for it bound to an o{lder,ther} version. + # ...we need to search for it bound to an o{lder,ther} version. Since + # we load all actions when the face is first references, this will be in + # memory in the known set of versions of the face. + (@faces[name].keys - [ :current ]).sort.reverse.each do |version| + break if action = @faces[name][version].get_action(action_name) + end end return action diff --git a/spec/lib/puppet/face/huzzah/obsolete.rb b/spec/lib/puppet/face/huzzah/obsolete.rb new file mode 100644 index 000000000..1f717ea2f --- /dev/null +++ b/spec/lib/puppet/face/huzzah/obsolete.rb @@ -0,0 +1,6 @@ +Puppet::Face.define(:huzzah, '1.0.0') do + action :obsolete do + summary "This is an action on version 1.0.0 of the face" + when_invoked do |options| options end + end +end diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb index 98887a778..8f9c349b6 100755 --- a/spec/unit/interface/face_collection_spec.rb +++ b/spec/unit/interface/face_collection_spec.rb @@ -97,6 +97,21 @@ describe Puppet::Interface::FaceCollection do end end + describe "::get_action_for_face" do + it "should return an action on the current face" do + Puppet::Face::FaceCollection.get_action_for_face(:huzzah, :bar, :current). + should be_an_instance_of Puppet::Interface::Action + end + + it "should return an action on an older version of a face" do + action = Puppet::Face::FaceCollection. + get_action_for_face(:huzzah, :obsolete, :current) + + action.should be_an_instance_of Puppet::Interface::Action + action.face.version.should == SemVer.new('1.0.0') + end + end + describe "::register" do it "should store the face by name" do face = Puppet::Face.new(:my_face, '0.0.1') -- cgit From 532c4f37e4f8289cf4a9871ebc0cb5086c2ba26a Mon Sep 17 00:00:00 2001 From: Daniel Pittman Date: Thu, 21 Jul 2011 16:43:16 -0700 Subject: (#7184) Load the core of obsolete versions of Faces. When we define an action on an older version of a Face, we must be sure to directly load the core of that version, not just define it with the external Action(s) that it had. Otherwise we break our contract, which is that any core Actions for a specific version will be available to your external Action for as long as we support that core version. Reviewed-By: Pieter van de Bruggen --- lib/puppet/interface/face_collection.rb | 32 ++++++++++++++++++++--------- spec/lib/puppet/face/1.0.0/huzzah.rb | 9 ++++++++ spec/lib/puppet/face/huzzah.rb | 2 ++ spec/unit/interface/face_collection_spec.rb | 27 ++++++++++++++++++++++-- spec/unit/interface_spec.rb | 9 +++----- 5 files changed, 61 insertions(+), 18 deletions(-) create mode 100755 spec/lib/puppet/face/1.0.0/huzzah.rb diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb index 868997b67..b1f6ba398 100644 --- a/lib/puppet/interface/face_collection.rb +++ b/lib/puppet/interface/face_collection.rb @@ -56,9 +56,7 @@ module Puppet::Interface::FaceCollection # # We use require to avoid executing the code multiple times, like any # other Ruby library that we might want to use. --daniel 2011-04-06 - begin - require "puppet/face/#{name}" - + if safely_require name then # If we wanted :current, we need to index to find that; direct version # requests just work™ as they go. --daniel 2011-04-06 if version == :current then @@ -90,18 +88,32 @@ module Puppet::Interface::FaceCollection latest_ver = @faces[name].keys.sort.last @faces[name][:current] = @faces[name][latest_ver] end - rescue LoadError => e - raise unless e.message =~ %r{-- puppet/face/#{name}$} - # ...guess we didn't find the file; return a much better problem. - rescue SyntaxError => e - raise unless e.message =~ %r{puppet/face/#{name}\.rb:\d+: } - Puppet.err "Failed to load face #{name}:\n#{e}" - # ...but we just carry on after complaining. + end + + unless version == :current or get_face(name, version) + # Try an obsolete version of the face, if needed, to see if that helps? + safely_require name, version end return get_face(name, version) end + def self.safely_require(name, version = nil) + path = File.join 'puppet' ,'face', version.to_s, name.to_s + require path + true + + rescue LoadError => e + raise unless e.message =~ %r{-- #{path}$} + # ...guess we didn't find the file; return a much better problem. + nil + rescue SyntaxError => e + raise unless e.message =~ %r{#{path}\.rb:\d+: } + Puppet.err "Failed to load face #{name}:\n#{e}" + # ...but we just carry on after complaining. + nil + end + def self.register(face) @faces[underscorize(face.name)][face.version] = face end diff --git a/spec/lib/puppet/face/1.0.0/huzzah.rb b/spec/lib/puppet/face/1.0.0/huzzah.rb new file mode 100755 index 000000000..00f20f096 --- /dev/null +++ b/spec/lib/puppet/face/1.0.0/huzzah.rb @@ -0,0 +1,9 @@ +require 'puppet/face' +Puppet::Face.define(:huzzah, '1.0.0') do + copyright "Puppet Labs", 2011 + license "Apache 2 license; see COPYING" + summary "life is a thing for celebration" + script :obsolete_in_core do |_| "you are in obsolete core now!" end + + script :call_newer do method_on_newer end +end diff --git a/spec/lib/puppet/face/huzzah.rb b/spec/lib/puppet/face/huzzah.rb index ab465d9e0..47cc3f32c 100755 --- a/spec/lib/puppet/face/huzzah.rb +++ b/spec/lib/puppet/face/huzzah.rb @@ -4,4 +4,6 @@ Puppet::Face.define(:huzzah, '2.0.1') do license "Apache 2 license; see COPYING" summary "life is a thing for celebration" script :bar do |options| "is where beer comes from" end + + script :call_older do method_on_older end end diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb index 8f9c349b6..514a624b1 100755 --- a/spec/unit/interface/face_collection_spec.rb +++ b/spec/unit/interface/face_collection_spec.rb @@ -35,7 +35,8 @@ describe Puppet::Interface::FaceCollection do end it "should attempt to load the face if it isn't found" do - subject.expects(:require).with('puppet/face/bar') + subject.expects(:require).once.with('puppet/face/bar') + subject.expects(:require).once.with('puppet/face/0.0.1/bar') subject["bar", '0.0.1'] end @@ -64,7 +65,8 @@ describe Puppet::Interface::FaceCollection do it "should return false if the face file itself is missing" do subject.stubs(:require). - raises(LoadError, 'no such file to load -- puppet/face/bar') + raises(LoadError, 'no such file to load -- puppet/face/bar').then. + raises(LoadError, 'no such file to load -- puppet/face/0.0.1/bar') subject["bar", '0.0.1'].should be_false end @@ -110,6 +112,27 @@ describe Puppet::Interface::FaceCollection do action.should be_an_instance_of Puppet::Interface::Action action.face.version.should == SemVer.new('1.0.0') end + + it "should load the full older version of a face" do + action = Puppet::Face::FaceCollection. + get_action_for_face(:huzzah, :obsolete, :current) + + action.face.version.should == SemVer.new('1.0.0') + action.face.should be_action :obsolete_in_core + end + + it "should not add obsolete actions to the current version" do + action = Puppet::Face::FaceCollection. + get_action_for_face(:huzzah, :obsolete, :current) + + action.face.version.should == SemVer.new('1.0.0') + action.face.should be_action :obsolete_in_core + + current = Puppet::Face[:huzzah, :current] + current.version.should == SemVer.new('2.0.1') + current.should_not be_action :obsolete_in_core + current.should_not be_action :obsolete + end end describe "::register" do diff --git a/spec/unit/interface_spec.rb b/spec/unit/interface_spec.rb index 4cb1f8743..4ff71ac3d 100755 --- a/spec/unit/interface_spec.rb +++ b/spec/unit/interface_spec.rb @@ -126,14 +126,11 @@ describe Puppet::Interface do end it "should try to require faces that are not known" do - pending "mocking require causes random stack overflow" - subject::FaceCollection.expects(:require).with "puppet/face/foo" - subject[:foo, '0.0.1'] + subject::FaceCollection.expects(:load_face).with(:foo, :current) + subject::FaceCollection.expects(:load_face).with(:foo, '0.0.1') + expect { subject[:foo, '0.0.1'] }.to raise_error Puppet::Error end - it "should be able to load all actions in all search paths" - - it_should_behave_like "things that declare options" do def add_options_to(&block) subject.new(:with_options, '0.0.1', &block) -- cgit