[Nut-upsdev] Many open green PRs in NUT, wanna merge?

Wed Apr 26 13:38:17 UTC 2017

On Apr 21, 2017, at 12:54 PM, Jim Klimov wrote:
> 
> Hi guys, I can't help noticing that many PRs seem good to go merged in NUT, so these improvements can either benefit the users instantly, or get exposed to more real-life testing and prove their worth or find issues to fix ;)
> 
> Are there any objections to going on a merging-spree, and boost the morale of external contributors along the way? Or, can I go on such a spree using reasonable judgment? :)

No objections in principle, but let's put this in context. We still have a number of seemingly simple patches on both the mailing lists and GitHub that need fairly extensive integration testing, or additional documentation. I don't think we have done much in the way of triage for the next release. A few issues have been added to the 2.7.5 milestone, but at this point, we probably want to pick a few of the PRs below, and defer the rest.

The libusb-1.0 branch is probably the most invasive change: https://github.com/networkupstools/nut/issues/300 <https://github.com/networkupstools/nut/issues/300> . While it has had some testing for problem models, I think it is probably the best candidate to push out for wider testing (as part of 2.7.5, say) - and we want to be careful that we don't add too many other changes that can't easily be switched off. We also haven't documented the differences between the libusb versions outside of the issue log.

Side note: I also would prefer that we not add more merge commits than actual commits (rebasing still gives the original author credit), but whatever. I think the more important thing is to not just blindly merge everything that has a green checkmark, given how our current CI systems do not consider system-level testing (and will never be able to verify anything more complicated than spelling/grammar in documentation).

I won't have time to look at the list below in any great detail until maybe this weekend, but perhaps we can add a milestone for topics like "documentation for 2.7.5"? I think it helps to have the issues/PRs sorted into smaller, related lists, especially when some of them partially supersede others.
> 
> I believe the following should have no major objections:
> * https://github.com/networkupstools/nut/pull/428 <https://github.com/networkupstools/nut/pull/428>
> * https://github.com/networkupstools/nut/pull/427 <https://github.com/networkupstools/nut/pull/427>
> * https://github.com/networkupstools/nut/pull/426 <https://github.com/networkupstools/nut/pull/426>
> * https://github.com/networkupstools/nut/pull/419 <https://github.com/networkupstools/nut/pull/419>
> 
> These may need a review, but seem functional already and can be expanded upon if needed (by other PRs later), or may just have fallen between the cracks and forgotten for no good reason:
> * https://github.com/networkupstools/nut/pull/422 <https://github.com/networkupstools/nut/pull/422>
> * https://github.com/networkupstools/nut/pull/423 <https://github.com/networkupstools/nut/pull/423>
> * https://github.com/networkupstools/nut/pull/418 <https://github.com/networkupstools/nut/pull/418> (this one I'd expand with verification that files are present and not empty before touching but LGTM otherwise)
> * https://github.com/networkupstools/nut/pull/349 <https://github.com/networkupstools/nut/pull/349>
> * https://github.com/networkupstools/nut/pull/276 <https://github.com/networkupstools/nut/pull/276>
> 
> * This doc update needs a bit of simple merge-conflict resolution but looks ok otherwise:  https://github.com/networkupstools/nut/pull/401 <https://github.com/networkupstools/nut/pull/401>
> * ...and this one originally had some concerns from @zykh and @clepple - but the PR structure was revised to break a big commit into many smaller commits (per-file) and excludes the QX document, so reviewers have a chance to point at dropping the other commits they really don't like. Or accept merging of the bunch, if no more strong opinions arise, and perhaps address other concerns in subsequent PRs ;)
> 
> These had objections not addressed since (but validity of objections may need revision, just in case - maybe the merges should not really be blocked?):
> * https://github.com/networkupstools/nut/pull/274 <https://github.com/networkupstools/nut/pull/274>
> 
> And this one began to have some feature and size creep, so I'd prefer to commit the foundations (which should work at least for the initially stated target of Linux systemd) and base some future point improvements separately... though maybe can schedule a bit of time to nail the Solaris SMF caveats first, at least before releasing 2.7.5 (though why should Penguins wait for that other OS's nuances to be solved? ;) they can look for bugs in their codepath already ;) ):
> * https://github.com/networkupstools/nut/pull/330 <https://github.com/networkupstools/nut/pull/330>
> 
Some of the feature creep commits were added while I was getting ready to go on vacation, so I can't say that I have a full understanding of what is going on there. From a sysadmin's perspective, I wonder if Base64 is the right way to do this? We should probably expose those conversions in a standalone tool (Jim, you mentioned something similar on 2017-03-07 <https://github.com/networkupstools/nut/pull/330#issuecomment-284793178> , or consider something with more traditional quoting semantics (systemd seems to do this for device nodes).

Note that I haven't tested this, so there may be some other way to view driver status in systemd that does not involve viewing Base64-encoded names.

> And finally there is the DMF branch, without a PR yet - but with hundreds of commits. It did not require maintenance (beside syncing from master) for several months now, while it works in our project on a daily basis, so I'd say it is also good to merge. It is waiting for Arno's precious and rare resource of time to review it for soon-to-be a year...

From the perspective of someone outside Eaton, this is a frustrating branch. It solves problems that many NUT users don't have, and while the latest few iterations have been cleaned up tremendously, there is a lot of technical debt on the configuration/compilation side that seems to be the result of rushing to add features initially. I suspect that there will be a few issues that crop up during packaging, and given the time it takes to rebuild and check .debs for the PPA on other branches, this is not something I look forward to doing in my free time.

Buildbot is up again, and as of ed4107d2, the DMF branch is failing across the board in distcheck-light.

Is there an integration test plan for this branch?

-- 
Charles Lepple
clepple at gmail

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20170426/dc75bb58/attachment.html>