> b33fb5e3 lock: alert the user about corrupted lock

[Comment only] This seems to have no relation to the change, and the referenced bug is already fixed?

> 31572946 refactor: implementation-agnostic interface to readurl

[Comment only] On what you describe in the commit message: in Python, I'm dubious about failures being presented by return codes instead of via exceptions. It's not the normal pattern, and thus seems error-prone in that a caller is more likely to fail to check and for that to be missed in future reviews.

> 296edca3 chore: rename repo GPG keys from -advantage to -pro
> 83808325 postinst: use for loop to rename gpg keys

[Comment only] I'm not sure why this is labelled as "chore". I don't subscribe to "semantic commit messages" but even then this is absolutely a user-visible change!

[Needs Information] Why are we renaming these keys at all? We're not renaming the package name, and the prefix /etc/apt/trusted.gpg.d/ubuntu-advantage- arguably makes it clear the these file belong to this package. And there are further complications: these are in /etc, so could be modified or removed by users, so we have to consider that type of case - at least to review to ensure that the maintainer scripts cannot explode. Further, even though mv_conffile is not appropriate in this case, it demonstrates the error rollbacks that are not implemented here, so a failure somewhere else in the postinst could lead to a user's system ending up in a broken state due to this part not being rolled back correctly, and that requires further thought, review and possible implementation. Is this worth the regression risk and review effort?

> 7c2f481b http: use pycurl for tls-in-tls requests

[Comment only]

Note that you are depending on ca-certificates (due to /etc/ssl/certs/ca-certificates.crt) but cannot Depend on it. The instruction "apt install python3-pycurl" will probably pull it in as a Recommends, but not necessarily if users have Recommends installation disabled (this is not uncommon in "minimal" server deployments). So it might be worth stating it explicitly.

IIRC this is also an issue for plain Python SSL. I remember it coming up but don't remember how we handled it. It might be worth verifying that there are tests to ensure that behaviour is reasonable when ca-certificates is not installed - both for the Python and pycurl cases.

> fead7d19 contract: support requiredPackages directive

[Needs Information] Under what circumstances are these directives provided? Just when a service is being enabled, or is it wider than this?

> c2a7f7ab landscape: support enabling and disabling landscape-client

[Comment only] I asked why is help_data.yaml in /etc, and understand that you are planning to move it out.

> 1162f56f esm: pin repositories to a higher priority using static files
> 21c28f36 esm: add comment to preference files
> 097d33bf chore: tweak messages in the preferences files

[Needs Fixing]

1. Could you please add a test to ensure that a regular archive package not from ESM remains *not* pinned (ie. priority 500)? Unless I'm missing something, I looked at features/attached_enable.feature and at sru/release-29/test-esm-pinning.sh and didn't see this, but a failure mode is that our pins are accidentally wider than intended.

2. Could we make it clearer to users disinterested in Pro so that they are reassured that this file has no effect? For example:

# This file is used by Ubuntu Pro and supplied by the ubuntu-advantage-tools
# package. It has no effect if Ubuntu Pro services are not in use since no
# other apt repositories are expected to match o=UbuntuESM.

# Pin esm-apps packages to a slightly higher value than the default,
# so those are preferred over a non-ESM package from the archive when the
# service is enabled.

> 8137d60d lintian: add overrides to some known warnings

[Needs Fixing in the future (not required for this SRU)]

These look fine, but some are wider than they need to be and the general lintian warning would still be useful to have. For example, I see the reason for possible-bashism-in-maintainer-script and you're correct to override it, but you could limit the scope of the override to just that line so that you still get warned about new bashisms in the future. You should be able to do this for every override that is specific to the instance, not the class.

> 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