Please review pull request #754: Bug/master/14200 face class names opened by (cprice-puppet)
Description:
This is basically just a re-introduction of
ce1e5555a847163e6448d1442a6d626957dac798, which was mostly
some cleanup and fixes for the help face. However, that commit
also (intentionally) broke backward compatibility for application class names;In 2.7.x, underscores in file names for puppet applications are expected to translate to underscores in the class name. So, e.g., the application defined in "resource_type.rb" is expected to have a class name of "Resource_type". However, that doesn't really match up with "standard" ruby or the expectations of ruby developers. They expect for "resource_type.rb" to translate to "ResourceType". The original commit introduced that, but did not provide backward compatibility support for the "_" class naming. This commit does everything that the original one did, but includes some hacks for backward compatibility with the underscore pattern.
- Opened: Fri May 11 00:02:24 UTC 2012
- Based on: puppetlabs:master (80c8f0a7b3a8b944388f506861c06efe6656020a)
- Requested merge: cprice-puppet:bug/master/14200-face-class-names (af309799c1fc7a2cc4756869f03e55e1cc8b8bc4)
Diff follows:
diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index e2ae01e..55e7ab0 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -219,18 +219,56 @@ def option_parser_commands
@option_parser_commands
end
- def find(name)
- klass = Puppet::Util::ConstantInflector.file2constant(name.to_s)
+ def find(file_name)
+ # This should probably be using the autoloader, but due to concerns about the fact that
+ # the autoloader currently considers the modulepath when looking for things to load,
+ # we're delaying that for now.
begin
- require ::File.join('puppet', 'application', name.to_s.downcase)
+ require ::File.join('puppet', 'application', file_name.to_s.downcase)
rescue LoadError => e
- puts "Unable to find application '#{name}'. #{e}"
- Kernel::exit(1)
+ Puppet.log_and_raise(e, "Unable to find application '#{file_name}'. #{e}")
end
- self.const_get(klass)
+ class_name = Puppet::Util::ConstantInflector.file2constant(file_name.to_s)
+
+ clazz = try_load_class(class_name)
+
+ ################################################################
+ #### Begin 2.7.x backward compatibility hack;
+ #### eventually we need to issue a deprecation warning here,
+ #### and then get rid of this stanza in a subsequent release.
+ ################################################################
+ if (clazz.nil?)
+ class_name = file_name.capitalize
+ clazz = try_load_class(class_name)
+ end
+ ################################################################
+ #### End 2.7.x backward compatibility hack
+ ################################################################
+
+ if clazz.nil?
+ raise Puppet::Error.new("Unable to load application class '#{class_name}' from file 'puppet/application/#{file_name}.rb'")
+ end
+
+ return clazz
+ end
+
+ # Given the fully qualified name of a class, attempt to get the class instance.
+ # @param [String] class_name the fully qualified name of the class to try to load
+ # @return [Class] the Class instance, or nil? if it could not be loaded.
+ def try_load_class(class_name)
+ begin
+ return self.const_get(class_name)
+ rescue NameError => ex
+ unless ex.message =~ /uninitialized constant/
+ raise ex
+ end
+
+ return nil
+ end
end
+ private :try_load_class
def [](name)
find(name).new
diff --git a/lib/puppet/application/certificate_request.rb b/lib/puppet/application/certificate_request.rb
index 8a26703..ba8c891 100644
--- a/lib/puppet/application/certificate_request.rb
+++ b/lib/puppet/application/certificate_request.rb
@@ -1,4 +1,7 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::CertificateRequest < Puppet::Application::IndirectionBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Certificate_request < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/certificate_revocation_list.rb b/lib/puppet/application/certificate_revocation_list.rb
index ee10775..218d2e6 100644
--- a/lib/puppet/application/certificate_revocation_list.rb
+++ b/lib/puppet/application/certificate_revocation_list.rb
@@ -1,4 +1,7 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::CertificateRevocationList < Puppet::Application::IndirectionBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Certificate_revocation_list < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/instrumentation_data.rb b/lib/puppet/application/instrumentation_data.rb
index 647fc36..ab431c3 100644
--- a/lib/puppet/application/instrumentation_data.rb
+++ b/lib/puppet/application/instrumentation_data.rb
@@ -1,4 +1,7 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::InstrumentationData < Puppet::Application::IndirectionBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Instrumentation_data < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/instrumentation_listener.rb b/lib/puppet/application/instrumentation_listener.rb
index 216e952..10e2893 100644
--- a/lib/puppet/application/instrumentation_listener.rb
+++ b/lib/puppet/application/instrumentation_listener.rb
@@ -1,4 +1,7 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::InstrumentationListener < Puppet::Application::IndirectionBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Instrumentation_listener < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/instrumentation_probe.rb b/lib/puppet/application/instrumentation_probe.rb
index 8f808c6..d6d3644 100644
--- a/lib/puppet/application/instrumentation_probe.rb
+++ b/lib/puppet/application/instrumentation_probe.rb
@@ -1,4 +1,7 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::InstrumentationProbe < Puppet::Application::IndirectionBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Instrumentation_probe < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/resource_type.rb b/lib/puppet/application/resource_type.rb
index 7842257..ac001f0 100644
--- a/lib/puppet/application/resource_type.rb
+++ b/lib/puppet/application/resource_type.rb
@@ -1,4 +1,7 @@
require 'puppet/application/indirection_base'
-class Puppet::Application::ResourceType < Puppet::Application::IndirectionBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Resource_type < Puppet::Application::IndirectionBase
end
diff --git a/lib/puppet/application/secret_agent.rb b/lib/puppet/application/secret_agent.rb
index 6d11be4..6bee284 100644
--- a/lib/puppet/application/secret_agent.rb
+++ b/lib/puppet/application/secret_agent.rb
@@ -1,6 +1,9 @@
require 'puppet/application/face_base'
require 'puppet/face'
-class Puppet::Application::SecretAgent < Puppet::Application::FaceBase
+# NOTE: this is using an "old" naming convention (underscores instead of camel-case), for backwards
+# compatibility with 2.7.x. When the old naming convention is officially and publicly deprecated,
+# this should be changed to camel-case.
+class Puppet::Application::Secret_agent < Puppet::Application::FaceBase
run_mode :agent
end
diff --git a/lib/puppet/face/help.rb b/lib/puppet/face/help.rb
index 2055f83..d42e4e5 100644
--- a/lib/puppet/face/help.rb
+++ b/lib/puppet/face/help.rb
@@ -171,9 +171,28 @@ def is_face_app?(appname)
# at least recognize that it's already done a 'require' for any individual app
# and basically treat it as a no-op if we try to 'require' it twice.
require "puppet/application/#{appname}"
- # Would much rather use "const_get" than "eval" here, but for some reason it is not available.
- # See #14205.
- clazz = eval("Puppet::Application::#{Puppet::Util::ConstantInflector.file2constant(appname)}")
+
+ expected_class_name = "Puppet::Application::#{Puppet::Util::ConstantInflector.file2constant(appname)}"
+
+ clazz = try_load_class(expected_class_name)
+
+ ################################################################
+ #### Begin 2.7.x backward compatibility hack;
+ #### eventually we need to issue a deprecation warning here,
+ #### and then get rid of this stanza in a subsequent release.
+ ################################################################
+ if (clazz.nil?)
+ expected_class_name = "Puppet::Application::#{appname.capitalize}"
+ clazz = try_load_class(expected_class_name)
+ end
+ ################################################################
+ #### End 2.7.x backward compatibility hack
+ ################################################################
+
+ if clazz.nil?
+ raise Puppet::Error.new("Unable to load application class '#{expected_class_name}' from file 'puppet/application/#{appname}.rb'")
+ end
+
clazz.ancestors.include?(Puppet::Application::FaceBase)
end
# This should probably be a private method, but for some reason it appears
@@ -181,4 +200,23 @@ def is_face_app?(appname)
# See #14205.
#private :is_face_app?
+ # Given the fully qualified name of a class, attempt to get the class instance.
+ # @param [String] class_name the fully qualified name of the class to try to load
+ # @return [Class] the Class instance, or nil? if it could not be loaded.
+ def try_load_class(class_name)
+ begin
+ # Would much rather use "const_get" than "eval" here, but for some reason it is not available.
+ # See #14205.
+ return eval(class_name)
+ rescue NameError => ex
+ unless ex.message =~ /uninitialized constant/
+ raise ex
+ end
+
+ return nil
+ end
+ end
+ #private :try_load_class
+
+
end
diff --git a/lib/puppet/util/logging.rb b/lib/puppet/util/logging.rb
index 8990e24..e5b329d 100644
--- a/lib/puppet/util/logging.rb
+++ b/lib/puppet/util/logging.rb
@@ -50,7 +50,7 @@ def format_exception(exception, message = :default, trace = true)
def log_and_raise(exception, message)
log_exception(exception, message)
- raise exception, message + "\n" + exception, exception.backtrace
+ raise exception, message + "\n" + exception.to_s, exception.backtrace
end
class DeprecationWarning < Exception; end
diff --git a/spec/unit/application_spec.rb b/spec/unit/application_spec.rb
index e01fc4d..807df98 100755
--- a/spec/unit/application_spec.rb
+++ b/spec/unit/application_spec.rb
@@ -38,17 +38,17 @@
end
it "should not find classes outside the namespace" do
- expect { @klass.find("String") }.to exit_with 1
+ expect { @klass.find("String") }.to raise_error(LoadError)
end
- it "should exit if it can't find a class" do
- @klass.expects(:puts).with do |value|
+ it "should error if it can't find a class" do
+ Puppet.expects(:err).with do |value|
value =~ /Unable to find application 'ThisShallNeverEverEverExist'/ and
value =~ /puppet\/application\/thisshallneverevereverexist/ and
value =~ /no such file to load|cannot load such file/
end
- expect { @klass.find("ThisShallNeverEverEverExist") }.to exit_with 1
+ expect { @klass.find("ThisShallNeverEverEverExist") }.to raise_error(LoadError)
end
it "#12114: should prevent File namespace collisions" do
-- 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.
