Code review comment for ~michal-maloszewski99/ubuntu/+source/backuppc:specify-canonical-path-ping6-focal

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Thanks for the MP, Michał.

The changes look good to me, but there's one minor nit: you have to run update-maintainer as well, because you're introducing Ubuntu-specific delta here.

I built the package locally and performed some tests inside a container. I installed the current version of backuppc, and then upgraded it to the version from your PPA. I've also modified the configuration file before performing the upgrade, and verified that dpkg will notify the user that there are changes that conflict with the existing version of the file, which is good.

I looked at the SRU template you wrote and I have a few comments:

- IMHO you can write the template directly in the bug.

- I believe you can rewrite the Impact section and make it more straightforward. The intention is to briefly explain that the current backuppc package on Bionic/Focal does not contain a valid setting for ping6, which causes problems for users whose backup hosts are using IPv6.

- I think you meant to write "hardcode" instead of "hard-core".

- Minor nit: we always try to use "ubuntu:focal" (or "ubuntu-daily:focal") as our LXD image. The "ubuntu:XYZ" images are produced by Canonical and include cloud-init and other technologies, so it's a good "dogfooding practice" to use them.

- I'd also suggest rewriting the "Where problems could occur" section. The text is looking a bit confusing as is. Also, the part about having a hardcoded value in the configuration file should be a bit more clear regarding the conflict that the user will face.

review: Needs Fixing

« Back to merge proposal