Code review comment for lp://staging/~jtv/launchpad/db-bug-766807

> Looks good, and wgrant's review was very thorough, so r=me with some
> small comments.

And yet, very observant ones. Thank you for that; it's one of the best ways to learn.

> + arguments = [
> + "-A",
> + "-d", self.distribution.name,
> + ]
> + for suite in self.getSuites():
> + arguments += ["-s", suite]
>
> If there are long forms of these arguments, would you be happy to use
> them so that it's easier to see what's happening?

Unfortunately I wouldn't. :( The -A one's more "meaningful" spelling is actually --careful-apt, which turns out to be a misnomer. It ought to be changed eventually. I could spell out "-d" in full ("--distribution") but that may seem incongruous.

In the longer term however we do intend to get rid of this ugliness altogether and call a proper python function rather than simulating a command-line invocation.

> + message = dedent("""\
> + You are receiving this email because you are registered in
> + Launchpad as a release manager for %s.
> +
> + The archive indexes for %s have been successfully created.
>
> It matters very little, but will MailController reflow the paragraphs
> in case the replacements are very long?

Probably not. A mail client might. This notification is a transitory solution; the real fix is to automate the full process so that the email is no longer needed.

> + controller = MailController(
> + from_addr, self.getMailRecipients(), subject, message)
> + if controller is not None:
> + controller.send()
>
> Is there any situation where controller will be None?

No, you're right. That check was from some previous state of the code, I think. Fixed.

> + def destroySelf(self):
> + """See `IDistributionJob`."""
> + Store.of(self.context).remove(self.context)
>
> Is this needed? It doesn't seem to be used (and isn't tested).

I couldn't find any use for it, but the interface specifies it (so I can't have my baseline tests for interface compliance without it) so I decided to test it properly. Also made the implementation clean up the underlying Job record as well.

> +def silence_publisher_logger():
> + """Silence the logger that `run_publisher` creates."""
> + getLogger("publish-distro").setLevel(FATAL)
>
> Does the test harness return the logger to its previous level after
> the test has run? If not, perhaps this could be changed into a
> Fixture.

I suspect that it won't. But in this particular case that might not be a bad thing: we'd stand to lose some test noise that we'd probably not bother to address otherwise.

> + def makeJob(self, distroseries=None):
> + """Create an `CreateDistroSeriesIndexesJob`."""
> + if distroseries is None:
> + distroseries = self.factory.makeDistroSeries()
> + job = removeSecurityProxy(self.getJobSource().makeFor(distroseries))
> + return job
> +
> + def getDistsRoot(self, distribution):
> + """Get distsroot directory for `distribution`."""
> + archive = removeSecurityProxy(distribution.main_archive)
> + pub_config = getPubConfig(archive)
> + return pub_config.distsroot
>
> Why is removeSecurityProxy() needed here? The need to unwrap these
> objects makes me think that their security proxies are not configured
> correctly.

Two very different answers for these two cases:

1. In makeJob, it's pure laziness. I inject a lot of fake methods and test a lot of methods that aren't in the interface. But it's not good practice, so I've now replaced it with removeSecurityProxy in just the places where it's actually needed.

2. The publisher config restricts access to admins.

> + def becomeArchivePublisher(self):
> + """Become the archive publisher database user (and clean up
> later)."""
> + self.becomeDbUser(config.archivepublisher.dbuser)
> + self.addCleanup(self.becomeDbUser, 'launchpad')
>
> I can't see that any other tests clean up after using becomeDbUser(),
> so perhaps you don't need to either.

Good point. I think I just put that in because it seemed cleaner. But as it turns out, leaving it out causes failures, perhaps because I also switch zope users.

> + def test_getSuites_ignores_suites_for_other_distroseries(self):
> + # getSuites does not list suites in the distributio that do not
>
> s/distributio/distribution/

Fixed.

> + def test_schedule_index_creation(self):
> ...
> + DistributionJobType.CREATEDISTROSERIESINDEXES,
>
> Old spelling.

Whoops, yes. Fixed now.

Thanks again!