Comment 30 for bug 2059756

I've completed the review of the rest of the adsys delta. This looks good to me; my only observation is that as part of the change of i18n handling libraries, there are two regressions in the localizability of strings:

internal/adsysservice/service.go: updateFmt := "%s" + gotext.Get(", updated on ") + "%s"
internal/cmdhandler/suggest.go: requireMsg := "%s " + gotext.Get("requires a valid subcommand")

Both of these introduce assumptions about the word order of the sentences which were not present before, making the new version less translatable than the previous version.

However, I will not block the SRU on this because I see that the only language adsys is currently translated to is French, which is not affected. Still, it is bad form to use concatenation of translated strings like this, which you appear to know because both of these are marked with 'fixme's in the code.

For the other issues with the packaging:
> - debian/prerm: thank you for catching this, I don't know how it slipped
> through, I guess it was all the false encouragement given by packages in the
> wild that employ the same (broken) prerm workflow. I've opened a PR to revert
> this upstream https://github.com/ubuntu/adsys/pull/1026 -- I don't think it
> adds much value to have this working (and it was not explicitly documented in
> the changelog either, so I'm happy with removing the broken code, as it never
> worked)

Yes, please revert this, thanks.

> - krb5/winbind as an alternative to sssd: this was explicitly requested by
> paying customers with specific requirements related to cross-domain forest
> setups which could not be satisfied by sssd directly -- it's documented (albeit
> in a terse manner, only mentioning winbind) in the 0.10.0 changelog

Ok, understood (and accepted for SRU).

> - addition of Depends: apparmor: opted for this because the apparmor policy
> manager subprocesses "apparmor_parser" (ref:
> https://github.com/ubuntu/adsys/blob/b20890f78bd55d5b5c90d77af61d17eb31f40b77/internal/policies/apparmor/apparmor.go#L82)
> -- happy to drop it if you think it's the best practice

Since you are invoking programs from this package directly, it's fine to leave this as-is.

> - additional of dependency on nfs-common: opted for a "Depends" here because
> adsys could apply a nfs mount policy which would fail hard if nfs-common were
> not installed -- I would've gone for "Recommends" in case of a soft warning

Ok, please leave this as-is.