my bugs & fixes

[Alex Harvey]

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

[Jeff McCune]

On Fri, Oct 12, 2012 at 3:05 AM, Alex Harvey <alexh...@gmail.com> wrote:

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 -

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

[Jeff McCune]

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

[Alex Harvey]

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

[Jeff McCune]

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

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.

To view this discussion on the web visit https://groups.google.com/d/msg/puppet-dev/-/a9VFiluyRkIJ.

To post to this group, send email to puppe...@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.

[Alex Harvey]

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

[Jeff McCune]

On Tue, Nov 6, 2012 at 5:29 PM, Alex Harvey <alexh...@gmail.com> wrote:

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?

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.

 

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.

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

[Alex Harvey]

On Thursday, November 8, 2012 8:27:39 AM UTC+11, Jeff McCune wrote:

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.

Will do.
 

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.

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.

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.

[Alex Harvey]

On Thursday, November 8, 2012 5:12:25 PM UTC+11, Alex Harvey wrote:

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.  [snip]

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