Code review comment for ~locnnil/ubuntu/+source/ntpsec:fix-ntpsec-apparmor

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

This looks good. A few comments:

a) Please also run update-maintainer, since this is adding a delta do a package that was so far in sync with debian. That will replace the maintainer in d/control to be ubuntu

b) Given the above, it would also be useful to send this change to debian, via a bug report to linuxptp[1], and/or a merge proposal in salsa[2]. I don't know which one the linuxptp maintainer prefers, but a bug report is always a good bet, and a PR would be a nice bonus. This is not a blocker for uploading to Ubuntu, though, and can happen in parallel. When/If debian takes the change, we can make the package a sync again.

c) Since we are so close to final freeze[3], I think this change needs a launchpad bug report against linuxptp as well, describing what is failing and how to trigger it. linuxptp is not seeded, and is in universe, and this is a bug fix, so I think uploading now is still fine, but a bug report will make the release team's job in checking what is going on much easier. Once you have that, don't forget to mention it in the d/changelog entry.

1. https://bugs.debian.org/cgi-bin/pkgreport.cgi?repeatmerged=no&src=linuxptp
2. https://salsa.debian.org/multimedia-team/linuxptp

review: Needs Fixing

« Back to merge proposal