Issue #17710 has been updated by Jeff McCune.
Status changed from Unreviewed to Accepted
Assignee deleted (Jeff McCune)
Target version set to 2.x
Alex Harvey wrote:
> Disclaimer - more experienced developers on the project may completely
> disagree with any or all of this. But for what it's worth the issues with
> the IP facts are as I see them -
>
> 1- A number of methods use case statements without default cases such that
> the code will need modification for each new kernel version that gets added.
> If the case statement is required at all (and I suspect it is) then perhaps
> the linux and BSD kernels ought to fall into the default case.
>
> This applies to the REGEX_MAP, and the get_all_interface_output and
> get_single_interface_output methods, in lib/facter/util/ip.rb; to
> Facter.add(:ipaddress) blocks in lib/facter/ipaddress.rb; to
> Facter.add("netmask") blocks in lib/facter/netmask.rb; and to
> Facter.add(:ipaddress6) blocks in lib/facter/ipaddress6.rb.
Agreed, this should be cleaned up.
> 2- This is a general problem with the facter code; there doesn't seem to be
> a clear rule about when to specify full paths to external commands and when
> not to. Or perhaps Luke had a clear rule that wasn't subsequently followed.
> In any case if we unset the path we can see that facter pushes
> /usr/sbin:/sbin onto the path internally -
In general, I think Facter should respect the PATH as set in the process
environment. Extenuating circumstances might motivate us to use fully
qualified paths, but in general PATH is the way to go.
> This makes the code vulnerable both to external commands like ifconfig being
> in unexpected locations (when the full path is hardcoded) and to code failing
> when $PATH isn't set in an expected way (i.e. to include /bin and /usr/bin).
>
> If developers agree with this I would be happy to go through the code and
> make this change.
I'm OK with changing things to respect PATH. Hard coding everything probably
isn't ideal.
> 3- Going back to the REGEX_MAP - is this really required at all? I think we
> could write a single regex for :ipaddress, :ipaddress6, :macaddress, and
> :netmask that handles all the known platforms. We could also avoid the
> problem of having no default case that way. We could then stub a whole range
> of ifconfig examples from all our platforms and properly test these using
> RSpec. I'd be happy to work on this too if developers agree.
I think we should break these facts apart as you mentioned, but I recommend
avoiding a top level case statement. Instead, I'd just define multiple
platform-specific facts that confine themselves accordingly.
> 4- All of the calls to external commands apparently would need to be
> rewritten using delegate methods so that their outputs can be stubbed without
> stubbing exec.
Yeah... This isn't too bad though.
> 5- Finally, all the tests in spec/unit/util/ip_spec.rb that stub
> :get_all_interface_output should be changed so that the outputs of the system
> commands instead are stubbed.
Agreed.
----------------------------------------
Refactor #17710: issues with IP facts
https://projects.puppetlabs.com/issues/17710#change-77893
Author: Alex Harvey
Status: Accepted
Priority: Normal
Assignee:
Category:
Target version: 2.x
Keywords:
Branch:
Affected Facter version: 1.6.14
--
You have received this notification because you have either subscribed to it,
or are involved in it.
To change your notification preferences, please click here:
http://projects.puppetlabs.com/my/account
--
You received this message because you are subscribed to the Google Groups
"Puppet Bugs" group.
To post to this group, send email to puppet-bugs@googlegroups.com.
To unsubscribe from this group, send email to
puppet-bugs+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-bugs?hl=en.
