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

Revision history for this message
Michał Małoszewski (michal-maloszewski99) 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.

Done

« Back to merge proposal