I think this is a good idea, but it's worth keeping in mind, as I've run into problems with this log-on-plugin-failure behavior before.
In particular, there seem to be a lot of cases where people are specifically looking for the thing that failed, so this behavior can be confusing. Like I said, I think this is right, but just keep an eye out for it over time as we get user feedback. On May 2, 2011, at 3:43 PM, Daniel Pittman wrote: > When we hit a syntax error in any face, a whole bunch of unrelated face things > would blow up in horrible ways. Stack traces for all... > > Now, instead, we catch that fault but specifically only in the face file > and report it through our error logs, then quietly ignore the face. > > Reviewed-By: Nick Lewis <[email protected]> > --- > lib/puppet/interface/face_collection.rb | 21 ++++++++------------- > spec/fixtures/faulty_face/puppet/face/syntax.rb | 8 ++++++++ > spec/unit/interface/face_collection_spec.rb | 19 ++++++++++++++++++- > 3 files changed, 34 insertions(+), 14 deletions(-) > create mode 100644 spec/fixtures/faulty_face/puppet/face/syntax.rb > > diff --git a/lib/puppet/interface/face_collection.rb > b/lib/puppet/interface/face_collection.rb > index 6e6afc5..baa4246 100644 > --- a/lib/puppet/interface/face_collection.rb > +++ b/lib/puppet/interface/face_collection.rb > @@ -10,21 +10,12 @@ module Puppet::Interface::FaceCollection > unless @loaded > @loaded = true > $LOAD_PATH.each do |dir| > - next unless FileTest.directory?(dir) > - Dir.chdir(dir) do > - Dir.glob("puppet/face/*.rb").collect { |f| f.sub(/\.rb/, '') > }.each do |file| > - iname = file.sub(/\.rb/, '') > - begin > - require iname > - rescue Exception => detail > - puts detail.backtrace if Puppet[:trace] > - raise "Could not load #{iname} from #{dir}/#{file}: #{detail}" > - end > - end > - end > + Dir.glob("#{dir}/puppet/face/*.rb"). > + collect {|f| File.basename(f, '.rb') }. > + each {|name| self[name, :current] } > end > end > - return @faces.keys.select {|name| @faces[name].length > 0 } > + @faces.keys.select {|name| @faces[name].length > 0 } > end > > def self.validate_version(version) > @@ -124,6 +115,10 @@ module Puppet::Interface::FaceCollection > 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 > > return get_face(name, version) > diff --git a/spec/fixtures/faulty_face/puppet/face/syntax.rb > b/spec/fixtures/faulty_face/puppet/face/syntax.rb > new file mode 100644 > index 0000000..3b1e36c > --- /dev/null > +++ b/spec/fixtures/faulty_face/puppet/face/syntax.rb > @@ -0,0 +1,8 @@ > +Puppet::Face.define(:syntax, '1.0.0') do > + action :foo do > + when_invoked do |whom| > + "hello, #{whom}" > + end > + # This 'end' is deliberately omitted, to induce a syntax error. > + # Please don't fix that, as it is used for testing. --daniel 2011-05-02 > +end > diff --git a/spec/unit/interface/face_collection_spec.rb > b/spec/unit/interface/face_collection_spec.rb > index 4358ef4..4ad8787 100755 > --- a/spec/unit/interface/face_collection_spec.rb > +++ b/spec/unit/interface/face_collection_spec.rb > @@ -22,7 +22,7 @@ describe Puppet::Interface::FaceCollection do > > after :each do > subject.instance_variable_set(:@faces, @original_faces) > - $".clear ; @original_required.each do |item| $" << item end > + @original_required.each {|f| $".push f unless $".include? f } > end > > describe "::prefix_match?" do > @@ -160,4 +160,21 @@ describe Puppet::Interface::FaceCollection do > end > end > end > + > + context "faulty faces" do > + before :each do > + $:.unshift "#{PuppetSpec::FIXTURE_DIR}/faulty_face" > + end > + > + after :each do > + $:.delete_if {|x| x == "#{PuppetSpec::FIXTURE_DIR}/faulty_face"} > + end > + > + it "should not die if a face has a syntax error" do > + subject.faces.should be_include :help > + subject.faces.should_not be_include :syntax > + @logs.should_not be_empty > + @logs.first.message.should =~ /syntax error/ > + end > + end > end > -- > 1.7.5 > > -- > 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. > -- There is nothing worse than aggressive stupidity. -- Johann Wolfgang von Goethe --------------------------------------------------------------------- Luke Kanies -|- http://puppetlabs.com -|- http://about.me/lak -- 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.
