[Patch/puppet 1/1] Fixes #2438, get major OS X version from Facter and replace Puppet::Error invocations with fail builtin

[Nigel Kersten]

Signed-off-by: Nigel Kersten <nig...@google.com>
---
.../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" %
- [@resource.class.name, @resource.name, detail]
+ fail("Could not set GeneratedUID for %s %s: %s" %
+ [@resource.class.name, @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" %
- [@resource.class.name, @resource.name, detail]
+ fail("Could not create %s %s: %s" %
+ [@resource.class.name, @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

[nigel kersten]

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.

[Luke Kanies]

+1

--
A cult is a religion with no political power. -- Tom Wolfe
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Luke Kanies]

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.

--
It is better to sleep on things beforehand than lie awake about them
afterward. -- Baltasar Gracian

[Brice Figureau]

On 23/07/09 18:02, Nigel Kersten wrote:
> Signed-off-by: Nigel Kersten <nig...@google.com>
> ---
> .../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)

My Facter (version 1.5) doesn't have this fact (but has
macosx_productversion).

Could it be possible to have a degraded mode, as right now I get tons of
failing (old) tests?

I know I should upgrade (and will), but supporting (not so) older
versions of Facter should be great, or at least produce a warning saying
that I should upgrade.
--
Brice Figureau
My Blog: http://www.masterzen.fr/

[Nigel Kersten]

We need to pick a minimum Facter version for 0.25.0 compatibility.

The reason I made this patch was because we had a bug in the Facter
code that essentially did the same thing, and fixing it in two
separate spots seems silly...

We've been doing this in install.rb, and I feel like that is the
correct place to check the Facter version. We should patch that when
we've decided what the minimum Facter version for 0.25.0 is.

It's the job of the packagers for various distros to ensure that
external dependencies are correct imho, and it seems like a waste of
effort to go through working out degraded modes for all the different
Facter features to me....

> --
> Brice Figureau
> My Blog: http://www.masterzen.fr/
>
>
> >
>

--
Nigel Kersten
nig...@google.com
System Administrator
Google, Inc.

[Luke Kanies]

I think that, at the least, it's reasonable to produce an upgrade
notice or something. Just having a failure because someone hasn't
upgraded isn't very good. Plenty of people use hand-rolled packages,
or git, or tarballs.

If it's reasonable to support the old version but produce a
deprecation notice, that would be best.

--
Writing is not necessarily something to be ashamed of, but do it in
private and wash your hands afterwards. --Robert Heinlein

[Nigel Kersten]

So I have a couple of options for the actual provider, and am happy to
take direction on any of them:

a) continue to duplicate the Facter code for major version inside the
provider without a Facter version warning.
b) continue to duplicate the Facter code for major version inside the
provider with a Facter version warning.
c) Fail with a deprecation warning if the Facter version is below
whatever version first provided this fact.

I still think we should be specifying a minimum Facter version for
Puppet 0.25.x somewhere, whether it be in the install.rb script, or in
the Puppet code, or both.

Whichever approach people are happy with, I'll make the tests do the
same thing I guess.

>
> --
> Writing is not necessarily something to be ashamed of, but do it in
> private and wash your hands afterwards. --Robert Heinlein
> ---------------------------------------------------------------------
> Luke Kanies | http://reductivelabs.com | http://madstop.com
>
>
> >
>

[Luke Kanies]

I would prefer option b here. I agree on specifying a minimum
version; you willing to also provide a patch to install.rb that checks
the version?

--
A motion to adjourn is always in order. --Robert Heinlein

[Nigel Kersten]

ok. Re-doing the patch with option b) and will submit a separate patch
to install.rb that checks.

>
> --
> A motion to adjourn is always in order. --Robert Heinlein
> ---------------------------------------------------------------------
> Luke Kanies | http://reductivelabs.com | http://madstop.com
>
>
> >
>