Thank you Steve, you raise several good points.
1. The excessive nesting.
I agree that it is annoying to read several layers of nested ifs. As an
individual issue, we don't think this is a blocker; however if we decide to
rewrite this function for this SRU for the below reasons, we can use the early
return like you suggest. If not, then we will simplify this for next release.
2. The check for deb-systemd-helper existing.
There's a longer story as to why we thought this check was a good idea at the
time, but the end result is that we were mistaken and it is superfluous. We
don't think this check hurts anything but it doesn't help either. We can remove
it in our next release unless you think it blocks this one.
3. The check for systemd existing.
This is present because we were copying a pattern that deb-systemd-helper uses
in its autosnippets. Whenever d-s-h appends a snippet that uses systemctl, it
makes this check first. For example, a snippet that is added to our postinst by
d-s-h:
# Automatically added by dh_systemd_start
if [ -d /run/systemd/system ]; then
systemctl --system daemon-reload >/dev/null || true
deb-systemd-invoke start ua-timer.timer >/dev/null || true
fi
# End automatically added section
However, it turns out our use of systemctl in this case is also
unnecessary. We don't need to run systemctl stop in this case because
ua-timer.timer will never have been started when this chunk of code
runs. Again, this shouldn't hurt anything. We can remove it in our next
release unless you think it blocks this one.
4. Running deb-systemd-helper enable is confusing.
The use of enable was intentional. We attempted to explain in the comment above
that line. The reason is that we want to take advantage of d-s-h's autosnippet
for enabling/updating config for ua-timer.timer which appends the following to
the bottom of postinst:
# Automatically added by dh_systemd_enable
# This will only remove masks created by d-s-h on package removal.
deb-systemd-helper unmask ua-timer.timer >/dev/null || true
# was-enabled defaults to true, so new installations run enable.
if deb-systemd-helper --quiet was-enabled ua-timer.timer; then
# Enables the unit on first installation, creates new
# symlinks on upgrades if the unit file has changed.
deb-systemd-helper enable ua-timer.timer >/dev/null || true
else
# Update the statefile to add new symlinks (if any), which need to be
# cleaned up on purge. Also remove old symlinks.
deb-systemd-helper update-state ua-timer.timer >/dev/null || true
fi
# End automatically added section
The chunk of code that we are talking about happens before this d-s-h
autosnippet, and the intent is to prevent this d-s-h snippet from running
"enable".
By running "enable" and then "disable" we update the d-s-h state such that
"was-enabled" will fail and so that snippet will not run "enable".
We acknowledge that this solution is confusing, to put it charitably. We've
evaluated a number of alternatives:
4.1. Straightforward simplification
deb-systemd-helper enable $UA_TIMER_NAME > /dev/null 2>&1 || true
deb-systemd-helper disable $UA_TIMER_NAME > /dev/null 2>&1 || true
This is the same solution but more consistent in its use of d-s-h for
tooling. We think this reads better but is not a significant improvement
on what is currently there. We'll simplify to this solution in the next
release.
4.2. Rely on d-s-h implementation details
touch
/var/lib/systemd/deb-systemd-helper-enabled/timers.target.wants/ua-timer.timer
This avoids needlessly enabling and disabling, but relies on the
implementation detail of d-s-h that it won't enable if this file is
present. It seems more fragile than the existing solution.
4.3 Rely on incidental behavior of update-state
deb-systemd-helper update-state ua-timer.timer >/dev/null || true
This command would set the d-s-h state appropriately to avoid enabling
the timer, but it is not the command's intended purpose. We think this
is at least as confusing as the existing solution and could be fragile
as well.
4.4 We stop relying on `dh_systemd_enable -pubuntu-advantage-tools ua-
timer.timer` and instead write the d-s-h enable code ourselves in
postinst (and the other operations it provides in preinst, prerm,
postrm).
This shifts all of the ua-timer.timer management responsibility from
d-s-h to us, the ua devs, and seems like an unnecessary burden just to
make this use case more straightforward.
A note on this use case: This snippet of code is only run for users who have
previously disabled ua-messaging.timer, which has only existed for ~6 months.
We expect the number of users this affects to be very low. With this snippet,
we are trying to carry their preference forward to the new timer, and it works
as written. We're not sure it is worth too much more effort to make the
solution more efficient or cleaner for this small edge case.
Overall, all of the issues you've pointed out are real, but we don't
think any negatively impact the package in a significant way that is
worth holding up this release. We can fix them in the next release.
Please let us know if you disagree.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1942929
Title:
[SRU] ubuntu-advantage-tools (27.2.2 -> 27.3) Xenial, Bionic, Focal,
Hirsute
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/ubuntu-advantage-tools/+bug/1942929/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
