On Jul 23, 2009, at 9:09 AM, nigel kersten wrote: > > Before Luke says it.... > > The whole directoryservice nameservice provider (and perhaps all of > nameservice in general?) needs a major major refactor, particularly so > that we can provide decent test coverage. > > I was hoping to get this done before 0.25.0, but I'm not sure I'll get > to it in time. > > The current implementation makes it really difficult to write clear > tests for, and I personally would rather spend the effort on > refactoring rather than writing tests, as once that is done, the tests > will be relatively trivial to write.
I think that makes sense. Testing code that wasn't written for tests really quite difficult; of course, this is one more reason to require tests with any new code -- if it is hard to test, it's probably hard to maintain. > > On Jul 23, 9:02 am, Nigel Kersten <[email protected]> wrote: >> Signed-off-by: Nigel Kersten <[email protected]> >> --- >> .../provider/nameservice/directoryservice.rb | 36 ++++++++ >> +----------- >> 1 files changed, 16 insertions(+), 20 deletions(-) >> >> diff --git a/lib/puppet/provider/nameservice/directoryservice.rb b/ >> lib/puppet/provider/nameservice/directoryservice.rb >> index 9daed17..f4c9d59 100644 >> --- a/lib/puppet/provider/nameservice/directoryservice.rb >> +++ b/lib/puppet/provider/nameservice/directoryservice.rb >> @@ -108,18 +108,14 @@ class DirectoryService < >> Puppet::Provider::NameService >> return @macosx_version_major >> end >> begin >> - product_version = Facter.value(:macosx_productversion) >> - if product_version.nil? >> - raise Puppet::Error, "Could not determine OS X >> version from Facter" >> - end >> - product_version_major = product_version.scan(/(\d+)\. >> (\d+)./).join(".") >> + product_version_major = >> Facter.value(:macosx_productversion_major) >> if %w{10.0 10.1 10.2 10.3}.include? >> (product_version_major) >> - raise Puppet::Error, "%s is not supported by the >> directoryservice provider" % product_version_major >> + fail("%s is not supported by the directoryservice >> provider" % product_version_major) >> end >> @macosx_version_major = product_version_major >> return @macosx_version_major >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not determine OS X >> version: %s" % detail >> + fail("Could not determine OS X version: %s" % detail) >> end >> end >> >> @@ -128,7 +124,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> dscl_output = execute(get_exec_preamble("-list")) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not get %s list from >> DirectoryService" % [ @resource_type.name.to_s ] >> + fail("Could not get %s list from DirectoryService" % >> [ @resource_type.name.to_s ]) >> end >> return dscl_output.split("\n") >> end >> @@ -228,7 +224,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> dscl_output = execute(dscl_vector) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not get report. command >> execution failed." >> + fail("Could not get report. command execution failed.") >> end >> >> # Two code paths is ugly, but until we can drop 10.4 >> support we don't >> @@ -283,7 +279,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> File.open(password_hash_file, 'w') { |f| >> f.write(password_hash)} >> rescue Errno::EACCES => detail >> - raise Puppet::Error, "Could not write to password hash >> file: #{detail}" >> + fail("Could not write to password hash file: #{detail}") >> end >> >> # NBK: For shadow hashes, the user AuthenticationAuthority >> must contain a value of >> @@ -305,7 +301,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> dscl_output = execute(dscl_vector) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not set >> AuthenticationAuthority." >> + fail("Could not set AuthenticationAuthority.") >> end >> end >> >> @@ -314,7 +310,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> password_hash_file = "#{@@password_hash_dir}/#{guid}" >> if File.exists?(password_hash_file) and File.file? >> (password_hash_file) >> if not File.readable?(password_hash_file) >> - raise Puppet::Error("Could not read password hash >> file at #{password_hash_file} for #...@resource[:name]}") >> + fail("Could not read password hash file at >> #{password_hash_file} for #...@resource[:name]}") >> end >> f = File.new(password_hash_file) >> password_hash = f.read >> @@ -358,7 +354,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> guid = >> guid_plist["dsAttrTypeStandard:#{@@ns_to_ds_attribute_map[:guid]}"] >> [0] >> self.class.set_password(@resource.name, guid, passphrase) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not set %s on %s[%s]: %s" % >> [param, @resource.class.name, @resource.name, detail] >> + fail("Could not set %s on %s[%s]: %s" % [param, >> @resource.class.name, @resource.name, detail]) >> end >> end >> >> @@ -389,7 +385,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> execute(exec_arg_vector) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not set %s on %s[%s]: >> %s" % [param, @resource.class.name, @resource.name, detail] >> + fail("Could not set %s on %s[%s]: %s" % [param, >> @resource.class.name, @resource.name, detail]) >> end >> end >> end >> @@ -416,8 +412,8 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> execute(exec_arg_vector) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not set GeneratedUID for >> %s %s: %s" % >> - [[email protected], @resource.name, detail] >> + fail("Could not set GeneratedUID for %s %s: %s" % >> + [[email protected], @resource.name, detail]) >> end >> >> if value = @resource.should(:password) and value != "" >> @@ -438,8 +434,8 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> execute(exec_arg_vector) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not create %s >> %s: %s" % >> - [[email protected], @resource.name, >> detail] >> + fail("Could not create %s %s: %s" % >> + [[email protected], @resource.name, >> detail]) >> end >> end >> end >> @@ -453,7 +449,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> execute(cmd) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not remove %s >> from group: %s, %s" % [member, @resource.name, detail] >> + fail("Could not remove %s from group: %s, %s" >> % [member, @resource.name, detail]) >> end >> end >> end >> @@ -466,7 +462,7 @@ class DirectoryService < >> Puppet::Provider::NameService >> begin >> execute(cmd) >> rescue Puppet::ExecutionFailure => detail >> - raise Puppet::Error, "Could not add %s to >> group: %s, %s" % [new_member, @resource.name, detail] >> + fail("Could not add %s to group: %s, %s" % >> [new_member, @resource.name, detail]) >> end >> end >> end >> -- >> 1.6.3.3 > > -- It is better to sleep on things beforehand than lie awake about them afterward. -- Baltasar Gracian --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
