Hi, I've reviewed 1.5.0-1 from mentors.debian.net. Note that I am a DM and not a DD so cannot upload this directly to Debian. My review was with my Ubuntu core dev hat as I can upload to Ubuntu directly and would like to do this by Ubuntu's feature freeze on Thursday, hopefully with a concurrent request for sponsorship for a Debian upload. I've only looked at the source and binary packages wrt. interactions with other packages and potential upgrade path issues. I've not actually tested the package - I assume it works. Review notes (blockers): Blocker for upload as there might be upgrade path issues later otherwise: ln -sf /usr/share/doc/kimchi/examples/kimchi.sub.nginx /etc/nginx/sites-available/kimchi (from postinst) will break policy https://www.debian.org/doc/debian-policy/ch-docs.html#s12.3 "Packages must not require the existence of any files in /usr/share/doc/ in order to function" and also the user expectation that a file in /etc can just be modified and normally conffile handling will occur. I suggest that you just install (duplicate the example in /usr/share/doc/kimchi/examples if you wish) to /etc as a regular conffile (so putting it in kimchi.install will do) instead. With just this fixed I will be happy to upload the package to Ubuntu and recommend to a DD that it is suitable for Debian (though it will still need the DD to review). Review notes (minor): The remaining issues I don't think need to be blockers for upload. These are just observations as I went through; I haven't thought about them in detail and for some of them the resolution may be that no action is required: package-contains-broken-symlink might be worth a lintian override as the symlink gets resolved when dependencies are fulfilled so is a false positive. I don't think the pedantic lintian warnings are an issue - if upstream don't sign releases then you can't verify them and you're already dealing with minified JS by minifying in the build and/or depending on the jquery package as appropriate. It might be worth writing a note for the Debian ftpmaster and/or Ubuntu archive admin explaining it though, perhaps in README.source? The postinst and postrm restart kimchid and nginx daemons. Does this intefere with dh_installinit #DEBHELPER# snippets at all? I see in the binary deb after build that it looks like it'll cause kimchi to be restarted twice on future upgrade, for example. I'm not sure how this could be improved though due to the need to restart nginx from kimchi's postinst. Maybe systemd might have some capability in this area? override_dh_auto_test is used to skip tests. It would be nice to be able to run any upstream tests at package build time, and/or have dep8 tests that can run upstream tests, but I appreciate that this may be difficult due to virt requirements. Upstream currently generate custom DH parameters at build time. This will not help with uniqueness in binary distributions like Debian and Ubuntu and also break reproducible builds. Instead this could be done at install time in the postinst, though that might result in entropy issues, or we could ship well-known parameters instead, or by some kind of user choice between the two. But I am advised by a colleague in Ubuntu security team that this isn't an immediate security issue as-is. I see debian/kimchid.init but this doesn't appear in the built binary. Should this be fixed or removed? Shipping just a systemd unit file is fine for Ubuntu; AIUI in Debian it's friendly to maintain it for users who choose a different init system. Maybe remove /usr/share/kimchi/doc/kimchid.8 as it is shipped in /usr/share/man/man8/kimchid.8.gz? Should /usr/lib/python2.7/dist-packages/kimchi/API.json be in /usr/share/kimchi instead? I'm not really sure about this one. It does seem unusual to have it where it is though.
Attachment:
signature.asc
Description: Digital signature