Comment 20 for bug 2059756

Revision history for this message
Steve Langasek (vorlon) wrote (last edit ):

I have begun reviewing the adsys upload in the jammy unapproved queue. The review is not yet complete, but I am sending my partial review notes for the desktop team to respond to.

First, I note that since this is a new upstream version, there are a number of bugs referenced in the changelog that are not going to go through SRU verification. Per https://wiki.ubuntu.com/StableReleaseUpdates#Bug_references_in_changelogs, please update the descriptions of these bugs to indicate this explicitly, so it is clear as part of the SRU review process that these are net SRU bugs that are missing the SRU bug template.

The bugs referenced in the .changes are:
LP: #2012371 LP: #2020682 LP: #2024377 LP: #2037270 LP: #2037271 LP: #2044112 LP: #2049061 LP: #2051363 LP: #2054445

Then, I have reviewed the changes to the Debian packaging in this package. I have prioritized this initially because as a rule, exceptions for new upstream versions of software do not extend to packaging changes since those pertain to distro integration and not upstream code. My observations:

- debian/prerm: this change is simply incorrect and clearly untested, because dpkg NEVER calls a prerm script with an argument of 'purge'; 'purge' is only ever given as an argument to a postrm script. This change to the prerm should be reverted, in devel and in the SRU in the queue; and brought back only after there is a verified implementation with a test case.
- apport hook changes: no runtime impact. accepted.
- addition of a new empty /var/lib/adsys dir: accepted.
- installation of additional adsys python module: accepted.
- lintian overrides change: no runtime impact. accepted.
- build dep on golang-1.22-go: golang-go in jammy is 1.18. accepted.
- added build-deps: accepted.
- krb5/winbind as an alternative to sssd: why? not discussed in the changelog and the integration of krb5+winbind directly, vs sssd, is strictly inferior.
- addition of Depends: apparmor - unusual; packages which integrate with apparmor do not in general depend on it. Basically irrelevant anyway since snapd does depend on apparmor and is mandatory. But I think this should be dropped in devel.
- additional of dependency on nfs-common: since nfs mounts are not used by default, and are overall uncommon in AD environments, I believe this better fits the policy definition of a Recommends rather than a Depends. However, adsys is opt-in and in either case will pull in nfs-common by default so I will not block on this.

I will follow up with any review comments on the upstream changes incrementally.