On Mar 29, 2010, at 7:53 AM, Nigel Kersten wrote:

So given all of the above, I still have a few questions.

1) If I'm not delegating :groups= and instead doing the logic in the
call itself, should I do the same thing for :groups as well for the
sake of symmetry?

No, I don't think this really matters, and in fact, I think it's cleaner to only deviate where you have to.

2) If we're trying to avoid Facter calls unless necessary, should I
stop doing the :uid :gid aliasing for 10.4 machines and instead define
sef.uid and self.gid and do the logic the same way?

Hmm. Well, I'd only make changes you need to make for 0.25, so I wouldn't for that release, but if this is for rowlf, and it's not a lot of effort, then yes, I guess I would like to see that.

3) And if we are doing this for :uid and :gid, should we also do this
for :uid= and .gid= for symmetry as per point 1 ?

Anope.

On Sat, Mar 27, 2010 at 2:55 PM, Luke Kanies <[email protected]> wrote:
On Mar 27, 2010, at 11:35 AM, Nigel Kersten wrote:

On Fri, Mar 26, 2010 at 5:55 PM, Nigel Kersten <[email protected]> wrote:

On Fri, Mar 26, 2010 at 5:22 PM, Luke Kanies <[email protected]> wrote:

I'd much prefer the groups= method have this behaviour in it, rather
than
have it done at loading time.

Everyone loads this util module but not everyone uses the groups=
method, so
just having the code in groups= will make testing/debugging much easier
and
just generally make the system simpler.

I don't think I quite get what you mean.

The groups= method gets delegated to Process.groups= ? It isn't
actually defined for this class at all normally.

Oh now I think I know what you mean. You're saying we should always
define self.groups= here and inside it decide whether or not to
delegate it to Process or stub it out for OS X 10.6 ?

I'm not entirely clear as to why that would be better though.

Yes, that's what I mean.

And the reason it's better is largely about when it's done - your original version of the code looks up Fact values when the file is parsed in the first place, regardless of whether 'groups' is ever called, and certainly
long before it normally would be called.

In contrast, if you have the logic inside the method, then you'll only
interact with Facter when necessary.

It's not a huge difference, but it really shows up when you're testing unrelaed code (and Facter gets called for no apparent reason, with stubs causing failures, etc.) and when you're trying to test this code - you'd currently have to stub Facter before you ever loaded this code, which is
basically impossible.

It's worth pointing out that there are one or two of these OS X special cases elsewhere in Puppet that cause weird failures - in fact, I ran into one of them yesterday - so anywhere we can keep things a bit simpler is
better.

It is probably worth caching the result of the Facter call, if this method
is used all the time.

On Mar 26, 2010, at 4:04 PM, Nigel Kersten wrote:


Signed-off-by: Nigel Kersten <[email protected]>
---
lib/puppet/util/suidmanager.rb |   15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/lib/puppet/util/suidmanager.rb
b/lib/puppet/util/suidmanager.rb
index a0a9178..99ed3a4 100644
--- a/lib/puppet/util/suidmanager.rb
+++ b/lib/puppet/util/suidmanager.rb
@@ -9,12 +9,25 @@ module Puppet::Util::SUIDManager
  to_delegate_to_process = [ :euid=, :euid, :egid=, :egid,
                             :uid=, :uid, :gid=, :gid, :groups=,
:groups
]

+    if Facter.value('kernel') == 'Darwin'
+        Facter.loadfacts
+ osx_maj_ver = Facter.value('macosx_productversion_major')
+        raise Puppet::Error, "OS X requires Facter >= 1.5.5" if
osx_maj_ver.nil?
+        # Process.groups= broken on 10.6
http://openradar.appspot.com/7791698
+        if osx_maj_ver == '10.6'
+            to_delegate_to_process.delete(:groups=)
+            def self.groups=(grouplist)
+                return true
+            end
+        end
+    end
+
  to_delegate_to_process.each do |method|
      def_delegator Process, method
      module_function method
  end

-    if Facter['kernel'].value == 'Darwin'
+ if Facter.value('kernel') == 'Darwin' and osx_maj_ver == '10.4'
      # Cannot change real UID on Darwin so we set euid
      alias :uid :euid
      alias :gid :egid
--
1.7.0.3

--
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.



--
I take my children everywhere, but they always find their way
back home. --Robert Orben
---------------------------------------------------------------------
Luke Kanies  -|-   http://puppetlabs.com   -|-   +1(615)594-8199

--
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.





--
nigel




--
nigel

--
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.



--
I don't know the key to success, but the key to failure is trying to
please everybody. -- Bill Cosby
---------------------------------------------------------------------
Luke Kanies  -|-   http://puppetlabs.com   -|-   +1(615)594-8199

--
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.





--
nigel

--
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 .



--
Smoking is one of the leading causes of statistics. -- Fletcher Knebel
---------------------------------------------------------------------
Luke Kanies  -|-   http://puppetlabs.com   -|-   +1(615)594-8199

--
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.

Reply via email to