Code review comment for ~ahasenack/ubuntu/+source/nfs-utils:jammy-nfs-utils-merge

Revision history for this message
Bryce Harrington (bryce) wrote :

For the NEWS entry, since the regex plugin is effectively a new feature, maybe it's not as necessary to explain why it was omitted, and instead just give mention that it's being kept an opt-in functionality available when the universe package is installed.

In any case, I'd also suggest a few sentences explaining what the regex plugin is, and why the reader might care to have (or not have) it. This email has some decent explanation text that could be cribbed from:

    https://www.spinics.net/lists/linux-nfs/msg77160.html

Indeed, one thing I'm a bit confused by is if I *want* the feature, do I just do `apt install libnfsidmap-regex` and then configure it in idmapd.conf? The NEWS file might be a good place to answer that.

Does the libnfsidmap-regex package provide man page info similar to what's being dropped with this merge? If it doesn't, then would it make sense rather than delete it, to instead just mention that a separate package needs installed first?

Do you think it would be worthwhile to mention the nfsconvert.py script in the section of the NEWS file explaining the config changes, so users can start converting? Or does it make more sense to wait until a conversion strategy is decided on?

The drops all LGTM. With such a big version jump it's no surprise that so much can drop, and I didn't dig far into those. Your explanations and rationale all sound sensible, and I verified all the delta items mentioned in the prior merge are addressed here.

The autopkgtests against the PPA pass:

Results from https://autopkgtest.ubuntu.com/results/autopkgtest-jammy-ahasenack-nfs-utils-merge/?format=plain:
  nfs-utils @ amd64:
    16.02.22 01:16:06 Log 🗒️ ✅ Triggers: ['nfs-utils/1:2.6.1-1~exp1ubuntu1~ppa9']
      local-server-client PASS ✅
  nfs-utils @ arm64:
    16.02.22 01:08:00 Log 🗒️ ✅ Triggers: ['nfs-utils/1:2.6.1-1~exp1ubuntu1~ppa9']
      local-server-client PASS ✅
  nfs-utils @ armhf:
    16.02.22 00:59:41 Log 🗒️ ✅ Triggers: ['nfs-utils/1:2.6.1-1~exp1ubuntu1~ppa9']
  nfs-utils @ ppc64el:
    16.02.22 01:06:25 Log 🗒️ ✅ Triggers: ['nfs-utils/1:2.6.1-1~exp1ubuntu1~ppa9']
      local-server-client PASS ✅
  nfs-utils @ s390x:
    16.02.22 01:01:38 Log 🗒️ ✅ Triggers: ['nfs-utils/1:2.6.1-1~exp1ubuntu1~ppa9']
      local-server-client PASS

I checked that it built fine locally.

Thanks for filing the bugs with the followup work. Can you also file one to add an apport hook for this package?

review: Needs Fixing

« Back to merge proposal