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

I removed the full stop, and made it use

    "%s %s" % (distribution.displayname, distroseries.displayname)

This will give us "Ubuntu Oneiric."  Using just "name" would make it "ubuntu oneiric," which is less nice for human consumption.


> 304     +class TestHelpers(TestCaseWithFactory):
> 305     +    """Test module's helpers."""
> 
> As above, this is all duplicated elsewhere. Please use the existing functions
> if possible.

I removed the test case.


> 601     +    CREATEDISTROSERIESINDEXES = DBItem(4, """
> 
> That's bordering on unreadable... maybe underscores are a good idea?

I agree...  it looked incongruous under DISTROSERIESDIFFERENCE an I'm not _entirely_ sure about spacing in DISTROSERIES, but I made it CREATE_DISTROSERIES_INDEXES.


Jeroen