Merge lp://staging/~jtv/launchpad/db-bug-766807 into lp://staging/launchpad/db-devel

Hi Jeroen,

I took another look at this as promised but I still don't trust myself to give it a proper review; I don't have the context to be able to understand it without a lot of hand-holding.

I think it would be a good idea for you to arrange for someone else on your squad to review this; they'll likely have the necessary context (and also won't have just returned from a two-week break).

Apologies for punting,

Graham

Looks mostly good, but a few comments:

43 +[create_distroseries_indexes]
44 +# The database role that these jobs will run in.
45 +# datatype: string
46 +dbuser: archivepublisher

Can you change JobCronScript to take the dbuser directly, rather than continuing the proliferation of pointless config sections?

79 + :param distroseries: A newly created `DistroSeries` that needs
80 + its archive indexes created.

The series may have been created some time ago; it was merely recently initialised.

135 +def get_addresses_for(person):
136 + """Get list of contact email address(es) for `person`."""

How does this differ from lp.registry.model.person.get_recipients?

145 +class CreateDistroSeriesIndexesJob(DistributionJobDerived):
146 + """Job to create a distroseries's archive indexes.
147 +
148 + To do this it runs publish-distro on the distribution, in careful mode.
149 + """

I'd prefer clarification that it operates only on the primary and partner archives, and only in careful *indices* mode.

190 + return "initializing archive indexes for %s" % self.distroseries.title

DistroSeries.title is pretty much useless... even __str__ is a much better idea.

197 + def runPublishDistro(self, extra_args=None):
198 + """Invoke the publish-distro script to create indexes.

As discussed last night, ew ew ew die. But OK.

216 + def getMailRecipients(self):
217 + """List email addresses to notify of success or failure."""
218 + recipient = self.distroseries.driver or self.distribution.owner

Both have a driver and an owner. Why driver of one and owner of the other?

223 + subject = "Launchpad has created archive indexes for %s." % (
224 + self.distroseries.name)

Email subjects don't tend to have full stops, and this doesn't identify the distribution. I'd go more along the lines of "Archive indices created for Ubuntu Oneiric". This whole thing also skips all our message rationale stuff, and it all seems like a fairly dubious idea, but I guess it will have to do for now.

Also using displayname and title here... distribution.name + series.name is probably better.

304 +class TestHelpers(TestCaseWithFactory):
305 + """Test module's helpers."""

As above, this is all duplicated elsewhere. Please use the existing functions if possible.

601 + CREATEDISTROSERIESINDEXES = DBItem(4, """

That's bordering on unreadable... maybe underscores are a good idea?

Thanks. Good stuff.

> 43 +[create_distroseries_indexes]
> 44 +# The database role that these jobs will run in.
> 45 +# datatype: string
> 46 +dbuser: archivepublisher
>
> Can you change JobCronScript to take the dbuser directly, rather than
> continuing the proliferation of pointless config sections?

This is how the job infrastructure likes us to do it now. Especially with the poor documentation we have, I'd rather stick to the pattern it expects than fight it.

> 79 + :param distroseries: A newly created `DistroSeries` that
> needs
> 80 + its archive indexes created.
>
> The series may have been created some time ago; it was merely recently
> initialised.

True. I was thinking "new" in the sense of "not having passed through this code yet," but I changed it as you suggest.

> 135 +def get_addresses_for(person):
> 136 + """Get list of contact email address(es) for `person`."""
>
> How does this differ from lp.registry.model.person.get_recipients?

Apparently it doesn't! Knowing that we had to have something like this I searched that file (and others) for the relevant words I could think of, but did not find get_recipients. That's a nice little weight loss program.

> 145 +class CreateDistroSeriesIndexesJob(DistributionJobDerived):
> 146 + """Job to create a distroseries's archive indexes.
> 147 +
> 148 + To do this it runs publish-distro on the distribution, in careful
> mode.
> 149 + """
>
> I'd prefer clarification that it operates only on the primary and partner
> archives, and only in careful *indices* mode.

Then the main thing that needs clarifying is the existence of a careful indices mode. I updated the help string for the option to say that, and reduced apt-ftparchive to the status of an example.

Changing the long option spelling seemed a bridge too far though. Another day.

> 190 + return "initializing archive indexes for %s" %
> self.distroseries.title
>
> DistroSeries.title is pretty much useless... even __str__ is a much better
> idea.

Okay: interpolating self.distroseries now.

> 197 + def runPublishDistro(self, extra_args=None):
> 198 + """Invoke the publish-distro script to create indexes.
>
> As discussed last night, ew ew ew die. But OK.

It's elsewhere as well. Sad, but there you go. We have a similar thing with LpQueryDistro, which is a LaunchpadScript but doesn't break the ugly pattern.

> 216 + def getMailRecipients(self):
> 217 + """List email addresses to notify of success or failure."""
> 218 + recipient = self.distroseries.driver or
> self.distribution.owner
>
> Both have a driver and an owner. Why driver of one and owner of the other?

The UI shows the distribution driver as Driver, but the distroseries driver as Release Manager.

I don't think a distroseries owner has much status in practice; moreover, if no release manager has been assigned for the series, I don't think it makes much sense to attach much credence to the series owner. The choice of distribution owner is more likely to be meaningful.

> 223 + subject = "Launchpad has...

StevenK also had a look at this and suggested I use the new, generic job runner. I made that change, which eliminated the need to execute the script in a separate process as an end-to-end test, and that in turn led to all sorts of other changes in the test.

Since the job runner wasn't documented, I added some descriptions that I hope will be helpful.

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.

> 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 = getPubConfi...