Code review comment for ~ahasenack/ubuntu/+source/samba:lunar-samba-merge-4175

  - samba/2:4.17.5+dfsg-2ubuntu1~ppa1
    + ❌ samba on lunar for amd64 @ 05.02.23 21:57:57 Log️ 🗒️
      • cifs-share-access PASS 🟩
      • cifs-share-access-uring PASS 🟩
      • python-smoke PASS 🟩
      • smbclient-anonymous-share-list PASS 🟩
      • smbclient-authenticated-share-list PASS 🟩
      • smbclient-share-access PASS 🟩
      • smbclient-share-access-uring PASS 🟩
      • reinstall-samba-common-bin PASS 🟩
      • samba-ad-dc-provisioning-internal-dns FAIL 🟥
    + ✅ samba on lunar for amd64 @ 05.02.23 23:19:08 Log️ 🗒️
    + ✅ samba on lunar for arm64 @ 05.02.23 20:51:10 Log️ 🗒️
    + ✅ samba on lunar for armhf @ 05.02.23 20:19:09 Log️ 🗒️
    + ✅ samba on lunar for ppc64el @ 05.02.23 20:36:31 Log️ 🗒️
    + ✅ samba on lunar for s390x @ 05.02.23 20:36:37 Log️ 🗒️

So it looks like the new test failed initially but passed on a re-trigger? Is that what you meant by it ran on an earlier version of the branch (so same version number, but changed contents)? I take it the 5 sec sleep is to allow the server to finish initializing, prior to the administrator password prompt?

More review comments on the new test inline below. I don't know Samba well enough to judge if the test is testing all the right things, but the logic looks great. The code looks correct too, my suggestions are mainly formatting and little tweaks and nits, so feel free to take or ignore as makes sense to you. While I hope you can incorporate some of my suggestions, I didn't spot anything crucially in need of fixing, so I'm marking this as approved. The rest of the packaging looks good, delta is carried as usual and commits all match to changelog entries.