Code review comment for ~lamoura/ubuntu/+source/ubuntu-advantage-tools:upload-29-mantic

On Thursday, August 17 2023, Robie Basak wrote:

>> 3b619ce9 postinst: change systemctl to deb-systemd-invoke
>
> [Needs Information]
>
> Did you do this because lintian told you to? deb-systemd-invoke is
> documented as only supporting start|stop|restart and I'm not sure
> policy-rc.d makes sense for daemon-reload or reset-failed anyway. But
> I tested this on Mantic and it seems to work. I'm OK with the change,
> but as it's in a less common upgrade path, do we have an ongoing test
> that validates that it will work into the future please? Alternatively
> this seems like a lintian bug that should allow "--system
> daemon-reload" and "reset-failed" and might be worth a bug report (and
> additionally a blanket "use deb-systemd-invoke" claim contradicts the
> command's own usage notice that only permits it for
> start|stop|restart). I see code to allow daemon-reload upstream
> already[1]. See also the scoped overrides I mentioned above.
>
> Please either: 1) defer the change; or 2) ensure an ongoing test exists for the purposes of exercising this code path (this might already exist; I didn't check; if you say there is one then that's good enough for me).
>
> I suggest also specific lintian overrides and a bug to lintian upstream.
>
> [1] https://salsa.debian.org/lintian/lintian/-/blob/ea05801918ed0e87824d89bf16a6ee166450b977/lib/Lintian/Check/MaintainerScripts/Systemctl.pm

Hey Robie,

Thanks for the extra review. This change was done because of a request
I made after building the package noticing the lintian warning.

My rationale here is that even though "deb-systemd-invoke" does not
explicitly document support actions other than start|stop|restart, there
is code in place to support deferring any other command to "systemctl"
directly:

  https://salsa.debian.org/debian/init-system-helpers/-/blob/master/script/deb-systemd-invoke#L157

This code (or other versions of it) has been there since the start, so
we should be covered when dealing with Xenial.

It is indeed arguable whether "deb-systemd-invoke" should be used only
for the actions mentioned above; the manpage's SYNOPSIS indicates that,
but the DESCRIPTION is more generic and doesn't impose the same limits.

Anyway, this is a really small change and I'm absolutely fine if Lucas
decides to revert it, although he also told me that they do have tests
to cover this code path, but I will let him elaborate more on that.

Thanks,

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14