Code review comment for ~danilogondolfo/ubuntu/+source/sudo:merge_mantic_lp2025655

FYI This kind of is the second run of https://code.launchpad.net/~danilogondolfo/ubuntu/+source/sudo/+git/sudo/+merge/443422

Sadly without pushing any tags (or anything else representing the old history) I have a hard time to compare and ensure this is a correct merge. Essentially I'll have to do the same effort again.
To explain - with you doing that I can compare your split to what is in Ubuntu, if that is the same I can compare what you have put on top of latest Debian with the delta we had on top of the former Debian.
Just wanted to explain why it helps to provide your git history of the old delta :-)

Ok, this is messy enough - I can give you a few comments for things to

0. Please provide the split delta on top of current Ubuntu as it would help.
I've seen that 1.9.13p3-1ubuntu1 is from you as well, shouldn't that be somewhere.
I found this branch and it is split \o/
So that is what I'll use.

---

1. In the old changelog you had:
      - debian/control:
        + Drop Build-Conflicts on fakeroot (<< 1.25.3-1.1ubuntu1)
        (for context see LP 1915250)

In the commit this was actually the change
-Maintainer: Sudo Maintainers <email address hidden>
+Maintainer: Ubuntu Developers <email address hidden>

Now this is confusing.

I see that the new merge has a proper "Update maintainer" commit. (f3d882bd)

And now that I found all that I realize why it isn't mentioned in changelog.

Other people might be confused and run into a rabbit hole here.
You should IMHO mention this in the changelog like for example:
  20 * Dropped changes
  21 - Drop Build-Conflicts on fakeroot (<< 1.25.3-1.1ubuntu1)
  22 [ This wasn't in the former Ubuntu version, just mentioned
  23 in the changelog by accident ]

---

2. In new changelog, but not mentioned as added:
      - debian/tests/04-getroot-sssd:
        + wait for 2 seconds before trying to access the slapd daemon.
          In some situations, the next command (ldapmodify) runs before
          the service is ready.

Prepend this by something like
  * Added changes
Because without it is listed under "Remaining" which isn't true.

---

3. the fix of debian/tests/04-getroot-sssd itself

There is no bug reference, no nothing.
I have to assume that the test failed autopktest.
You have to understand any "sleep x" causes allergic reactions.
What if tomorrow the machine is slower, then it fails again.

It would be much better (and I'd ask to add this instead) to do

# 1. Start the server as you already do
slapd -h "ldaps:/// ldapi:///" -g openldap -u openldap -F /etc/ldap/slapd.d
# 2. check if it is ready
$ ldapwhoami -Y external -H ldapi:///

This is not doing anything (no-op) but will deliver

root@m:~# ldapwhoami -Y external -H ldapi:///
ldap_sasl_interactive_bind: Can't contact LDAP server (-1)
root@m:~# echo $?
255

root@m:~# ldapwhoami -Y external -H ldapi:///
SASL/EXTERNAL authentication started
SASL username: gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth
SASL SSF: 0
dn:gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth
root@m:~# echo $?
0

So with that you can differentiate good/bad case before the test goes on.
Do that in a loop with a much longer overall timeout.
Then you can be sure it is not failing again another day / another platform.

---

4. commit messages

You used to have commit messages that you could directly re-use as d/changelog entries in
1.9.13p3-1ubuntu1
But now you have changed them all, example:

1: 29450519 ! 1: 7089bfc9 - debian/sudo[-ldap].manpages: install man/man8/sudo_root.8
    @@ Metadata
     Author: Danilo Egea Gondolfo <email address hidden>

      ## Commit message ##
    - - debian/sudo[-ldap].manpages: install man/man8/sudo_root.8
    + debian/sudo[-ldap].manpages: install man/man8/sudo_root.8

      ## debian/sudo-ldap.manpages (new) ##
     @@

This isn't strictly required, but I personally prefer the former form.
As then people can just re-merge with tooling without any manual interaction.

---

I didn't find more in
$ git range-diff pkg/import/1.9.13p3-1..OLD-UBUNTU pkg/import/1.9.13p3-3..danilogondolfo/merge_mantic_lp2025655

So it should be good after fixing the above.

Please ping back the next patch pilot or me or Athos for re-review once this was adressed.