Code review comment for ~paelzer/ubuntu/+source/tftp-hpa:merge-5.2+20150808-1.1ubuntu1-GROOVY

Revision history for this message
Bryce Harrington (bryce) wrote :

* Changelog:
  - [√] old content and logical tag match as expected
  - [√] changelog entry correct version and targeted codename
  - [x] changelog entries correct
    + See below
  - [√] update-maintainer has been run

* Actual changes:
  - [-] no upstream changes to consider
  - [-] no further upstream version to consider
  - [√] debian changes look safe

* Old Delta:
  - [-] dropped changes are ok to be dropped
  - [?] nothing else to drop
    + See below
  - [?] changes forwarded upstream/debian (if appropriate)
    + See below

* New Delta:
  - [√] no new patches added
  - [-] patches match what was proposed upstream
  - [-] patches correctly included in debian/patches/series
  - [-] patches have correct DEP3 metadata

* Build/Test:
  - [√] build is ok
  - [√] verified PPA package installs/uninstalls
  - [-] autopkgtest against the PPA package passes
  - [-] sanity checks test fine

Very minor nitpick, but in the changelog entry you have "(LP #1228340)(LP #1342580)", which I see more conventionally shown like "(LP #1228340, #1342580)" or sometimes "(LP #1228340, LP #1342580)" (I guess for auto-linking purposes).

I notice in 5.2+20150808-1ubuntu4 there was introduced a fix for (LP: #1855490) but this is not mentioned in the merge either as retained delta or drop. I'm guessing it was folded in to the IPv4/IPv6 item but I wonder that at least the bug # should be included with the other two bug #'s?

Other question relates to the upstreaming of the delta to Debian. In checking the two bugs, and the associated LP #1448500, I see there was a little discussion with Debian at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=771441 but it's unclear to me if all the changes were presented to Debian and if so, why they need to stay as Ubuntu delta. This might be worth documenting in the merge changelog if you know it offhand. If you don't know offhand, then for expediency can skip but this might be worth adding a Trello card to follow up on for a future merge.

There appears to be no autopkgtests for this package (c.f. LP #1679407), but install/remove from the PPA works.

review: Needs Fixing

« Back to merge proposal