Hello all,

I am sure people are busy but I thought I'd put it out there that I've been
waiting quite a while for a number of bugs & some code to be reviewed -

#14674 - raised as a bug, but I think really a refactor, which I need
implemented before fixing some HPUX facts - waiting on code review
#13535 - a fix for the uptime fact on AIX, HPUX & legacy Solaris, includes
a large number of RSpec tests - waiting on code review

#11612 - waiting on feedback in the Redmine ticket
#11609 - waiting on feedback in the Redmine ticket

#16506 - waiting on bug I reported to be reviewed/accepted
#16511 - waiting on bug I reported to be reviewed/accepted
#16526 - waiting on bug I reported to be reviewed/accepted
#16527 - waiting on bug I reported to be reviewed/accepted

Best wishes,
Alex Harvey

Alex,

We sure have been, it was nice to finally get 3.0.0 released during Puppet
Conf this year.  I'll take a look at as many of these issues as I can
today.  Thank you for your contribution and for your patience.

-Jeff

Alex,

I'm sorry I didn't make any progress on these issues on Friday or over
the weekend.  I'm currently reviewing them and will be working through
them in the order listed.  I'm jmccune on #puppet and #puppet-dev, if
there's anything that comes up that you think will be better worked
out via direct communication, please mention my nick directly.

Thanks,
-Jeff

Hi Jeff,

I've seen all your updates - thanks!  In general I already have fixes ready
for quite a few of them - I just needed them accepted/reviewed before I
could proceed to submit.  In a few other cases I'll update the tickets with
more specific comments.

Best regards,
Alex

Alex,

Thanks again for pinging us about these issues.  Doing so really helps me
make sure I'm not letting valuable contributions slip through the cracks.

At this point, I think all of these issues are resolved and are going to be
released in the next versions of Puppet and Facter.  Is this the case, or
is there something I've overlooked and need to address at this time?

Thanks again,
-Jeff

Hi Jeff,

Thanks for all your hard work and also for patiently explaining so much of
the development process to me.  All of the issues I raised are resolved.

Keep in mind however that #11612 is really a ticket about multiple issues
and adding processorX facts was only one part.  Maybe it should be closed
and raised as separate tickets?  Still to do, add physicalprocessorcount
(this is also a headache on HP-UX) and add Hongbo Hu's bugfix for dealing
with HP-UX NICs that have an asterisk after them.  This could probably be
put in straight away - but the question is are we going to use this
opportunity to write proper unit tests for ip.rb?  That might take me,
personally, too far afield.

See my comment at
https://projects.puppetlabs.com/issues/11612#note-31

Best regards,
Alex

Yes, I think we should leave 11612 as it is.  Merged pending release is a
state that is considered "closed," but it's a soft form of being closed.
 The ticket changes to the closed state when the change is actually
released.  Would you mind creating additional tickets for the following
"still to do" items?  One ticket for each actionable issue will help me
streamline the changes through our system.

I'd be happy to write the unit tests for ip.rb, but I do this sort of work
when I have the opportunity, which is not all that frequently.  If you
could isolate the potential blocker of unit tests in it's own issue, it'll
help keep the other issues streamlined.

I'm also happy to file these tickets, but I think it will be faster and
more clear if they come from you since I'm doing quite a bit of context
switching across facter, puppet and stdlib at the moment.

-Jeff

Will do.

Happy to help out where I can.   I'm still new to this so here's the
problem as I see it -

The spec test for the interface list on HP-UX is -

  it "should return a list three interfaces on HP-UX with three interfaces
multiply reporting" do
    hpux_netstat = my_fixture_read("hpux_netstat_all_interfaces")
    Facter::Util::IP.stubs(:get_all_interface_output).returns(hpux_netstat)
    Facter::Util::IP.get_interfaces().should == ["lan1", "lan0", "lo0"]
  end

The fixture hpux_netstat_all_interfaces is, surprisingly, not the output of
netstat -in on HP-UX, but rather what the author believed the
get_all_interface_output method would return.

Now, here is the get_all_interface_output method -

  def self.get_all_interface_output
    case Facter.value(:kernel)
    when 'Linux', 'OpenBSD', 'NetBSD', 'FreeBSD', 'Darwin', 'GNU/kFreeBSD',
'DragonFly'
      output = %x{/sbin/ifconfig -a 2>/dev/null}
    when 'SunOS'
      output = %x{/usr/sbin/ifconfig -a}
    when 'HP-UX'
      output = %x{/bin/netstat -in | sed -e 1d}
    when 'windows'
      output = %x|#{ENV['SYSTEMROOT']}/system32/netsh.exe interface ip show
interface|
      output += %x|#{ENV['SYSTEMROOT']}/system32/netsh.exe interface ipv6
show interface|
    end
    output
  end

The supplied fixture for HP-UX is the output of netstat -in |sed -e 1d -

root@myhost:(ip)> cat hpux_netstat_all_interfaces
lan1      1500 192.168.100.0   192.168.100.182 12964   0     900     0     0
lan0      1500 192.168.100.0   192.168.100.181 12964   0     715     0     0
lo0       4136 127.0.0.0       127.0.0.1       98      0     98      0     0

However the author has not considered the case of netstat -in output that
has asterisks after the interface as in -

root@myhost:()> netstat -in |sed -e 's/[0-9]/X/g'
Name      Mtu  Network         Address         Ipkts   Ierrs Opkts   Oerrs
Coll
lanX*     XXXX XX.XX.XX.X      XX.XX.XX.X      XXX     X     XXX     X     X
lanX      XXXX XXX.XX.X.X      XXX.XX.X.X      XXXXXXXXXX X     XXXXXXXXX
X     X
loX       XXXX XXX.X.X.X       XXX.X.X.X       XXXXXXX X     XXXXXXX X     X

And as Hongbo Hu showed, you can solve the problem easily enough by simply
extending the sed command to delete not only the first line, but also any
asterisks.

From a unit test perspective, however, the code would need to be changed in
order to reproduce the failing behaviour.

My feeling is we would need to rewrite this code with shim methods for the
commands that are executed externally, collect a good sample of outputs
from the various platforms that the code executes on, and then stub all
these calls to external commands in completely new unit tests.  

Next problem is that the code uses a big case statement of the kind Andy
said he doesn't like.  So, I guess we would also want to completely rewrite
the method - and that would affect all platforms.

Finally, the whole of ip.rb and ip_spec.rb is written in the same way.  So,
my conclusion is - since the code's working it's probably low priority and
when we really have lots of spare time then the whole thing needs to be
rewritten, code, unit tests and all.  So it's not obvious to me that it's
worth the effort of changing this method and implementing spec tests if,
really, the whole thing needs to be rewritten.

People with more experience on the project might come to a different
conclusion of course.

On having another look at this I must say that 'whole of ip.rb and
ip_spec.rb' is probably a harsh and unfair overstatement. :)