Comment 112 for bug 1640978

TL;DR
- LGTM a few questions for clarification, but no nacks

----

I took logs for everything non trivial that I did (wonder about) on review.
+ = LGTM
? = ok, but I'd have a question
x = nack, this should be different

python-acme:
? rebase for 0.22.2-1ubuntu0.1
  - The bug #1777205 should get a xenial task before upload
  - and then the bug be verified on the SRU
  - please remember to buildpkg with -v0.4.1-1
+ changelog tweaks
+ backport version string
  Double ubuntu in the version seems odd, but it seems correct.
  It is not part of [2] and an even more special case of [1]
x Add SRU explanation to changelog
  The version is 0.22.2 the entry calls it "version 0.23 in 16.04"
  Seems wrong, either fix or explain why it is that way.
  I see that all others e.g. python-certbot is at 0.23 for real
  will that mismatch be a problem?

python-certbot:
+ recommends certbot
? dh_clean ok but why, would it hurt?
? pkg-info.mk ok but why, would it hurt?
? cli.ini
  That is the one to keep the logrotations as is (ok)
  I thought from our talk Xenial did that as well, but it didn't
  There is nothing about it in pkg/ubuntu/xenial-devel:debian/rules
  nor in pkg/import/0.14.2-1:debian/rules
  I guess the old default was different, which is ok.
  But do we now want/need any conffile cleanup in >=Bionic?
  This would be independent to this SRU, but I wanted to ask.
+ build rules
+ install init no-op
? Restore original timer service installation
  The "original" used the older dh_systemd_enable/dh_systemd_start
  I think your's work only with newer debhelper, it is not "the original"
  I just found you clean that up later - would have been nice to squash those two :-/
+ backport version string (that one I like more)
? use dh_systemd
  The old package used install -D --mode=644 for the timer
  any reason you have chosen mkdir + cp instead ?
  That commit also adds whitespace damage above "# Install certbot.timer"
+ doc paths
+ version/changelog changes
? Add SRU explanation to changelog
  you might mention changes to d/rules and d/compat to make the backport working here

python-letsencrypt:
+ Remove redundant incorrect d/README.source
? further changes to changelog ( I have a general statement about those below)
? It is not clear to me how that "shimming" takes place, extending the changelog on that would be useful as well.

python-josepy:
? changelog/version bumps in general ok
  You mentioned the new certbot.timer and such in python-letsencrypt (which doesn't hold the timers)
  Please choose:
  - Why did you mention it there?
  - Why didn't you mention it here?
  I assume it is because of the package takeover letsencrypt->certbot.
  But really, that could use some words (more about that below already)

python-letsencrypt-apache
+ cleanup
+ changelog
+ version bumps

python-certbot-apache:
+ empty dir
+ VCS
+ cleanup (again ok but why)
+ doc building
+ version string changes
+ doc-base paths
+ SRU explanation (nothing more than what was said already)

I just realized that the summary changelogs all say: "Log rotation is switched to logrotate via /etc/logrotate.d/certbot"
This whole section could need more why it was done and where the old/new tunables are (cli.ini belonge to package X, the logrotate belongs to package Y - how it was before and now).
And since you carry that in all packages, copy it to all of them once you have a more explaining text.

There were quite a few which took me some minutes to think, but eventually LGMT but some cleanups.

You have done many cleanups which are correct (dh_clean, readme.source, pkg-info.mk, ...).
Those seem good for Disco onwards, but not necessary for an SRU.
They correct, but not necessary, but they increase the amount of code to review or did you only pick those which were ok in Xenial and comparin X->B would have showed up as bad delta?
If that was the intention I'm sure you have the thanks of the SRU reviewer :-)

In general the dependencies in d/control seem a bit soft, with all the renames I had expected more breaks/replaces.
Have you checked in the build logs that the generated python/misc:depends are as you'd want them?

All of the above is easy to clean up, so it should not be a huge bump in bringing this to the SRU queue.

Note: I'd recommend in general to drop the PPA string from git. Lessons-learned of someone that had to ask to cancel an SRU with ~ppa in the name :-) Keep it in "git stash" instead.

[1]: https://lists.ubuntu.com/archives/ubuntu-devel/2018-July/040406.html
[2]: https://wiki.ubuntu.com/SecurityTeam/UpdatePreparation