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