Conversation
|
Ah excellent - I wasn't aware of Facter::Util::Resolution.exec so I'll use that. As I said in the comments on my patch, I've just reaslised it can be refactored much cleaner if I use the same methodology as the ipv6 variant (stick the regexp in a separate function) - we should be able to get a nice clean solution between these :-) |
|
Sure! I agree, I think the whole thing needs a larger overall reactor. I was trying to leave this patch only in the context of the bug on Linux. Perhaps a refactor ticket should be opened for this fact in all its various forms. |
|
Tom, Please clean up the commit comment to match the style in the README - take a look at recent commits for an example. In terms of code, my only concern is whether or not "ip addr" is in fact sufficiently cross-platform. If we can confirm it is (did you run the tests on multiple OSes?) then I think this is good stuff. |
|
Sorry, another comment - I would rebase to make one commit as well. |
This commit changes the version of the ipaddress fact that is confined with Linux to use the 'ip' command.Recent versions of net-tool have stated that ifconfig is obsolete. It also states to rely on 'ip addr' as a replacement. The output was also changed to be more like that of the 'ip addr' output. This commit also changes the fact to use 'Facter::Util::Resolution.exec'. The 'ip' command is left unqualified since the recent changes to 'Facter::Util::Resolution.exec' include functionality that mimics the 'which' command but does not need to fork. The switch to 'Facter::Util::Resolution.exec' also allows for unit tests, which are part of this commit.
|
Eric, I did the rebase. Also, ifconfig is being made obsolete by iproute2, which has been available (and should have been included in systems) for over a decade. |
spec/unit/ipaddress_spec.rb
Outdated
There was a problem hiding this comment.
Instead of doing Facter::Util::Config.stubs(:is_windows?).returns(false) and Facter::Util::Resolution.stubs(:exec).with('uname -s').returns('Linux') it would be better, and a little safer for testing, just to do Facter.fact(:kernel).stubs(:value).returns("Linux").
This commit changes the stubbing used in the spec test as recommended by Hailee Kenney. This new stubbing will remove a dependancy on the :kernel fact. Now, if the :kernel fact is broken, this test will still work for its original purpose, which is to test the :ipaddress fact.
|
Hkenny, I see exactly what you're talking about. It was dependent on the proper function of the :kernel fact. I've added a commit to change that. Thanks for the advice. This should probably be changed in the :ipaddress6 fact as that is what I based this unit test off of. |
|
@trlinkin Thanks for making that change! And thanks for pointing out the issue with the |
|
From the ticket: I don't think this can go in as-is. Unfortunately, while the We really need to prefer using it if available, but to fall back to ifconfig parsing when it can't be found. |
…t_update (CFACT-162) Update uptime regular expressions for additional formats
5 participants
I did not want to upstage the previous submitted solution to this problem. However, I did want to submit a change that supported unit testing. I did not fully qualify the 'ip' command since I see 'Facter::Util::Resolution.exec' now supports an internal 'which like' functionality that does not need to fork, and also checks in '/sbin' by default. I'll leave it up to others to convert and write tests for the other kernel types.