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.

[1]

+ 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?

[2]

+ 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?

[3]

+ controller = MailController(
+ from_addr, self.getMailRecipients(), subject, message)
+ if controller is not None:
+ controller.send()

Is there any situation where controller will be None?

[4]

+ 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).

[5]

+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.

[6]

+ 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.

[7]

+ 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.

[8]

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

s/distributio/distribution/

[9]

+ def test_schedule_index_creation(self):
...
+ DistributionJobType.CREATEDISTROSERIESINDEXES,

Old spelling.