Merge lp:~jtv/launchpad/bug-824499 into lp:launchpad

[Jeroen T. Vermeulen]

All-derived-distros option for publish-ftpmaster.

= Summary =

This adds an option to publish-ftpmaster to, rather than publishing a single distribution named on the command line (and let's face it, that's probably going to be Ubuntu) loops over all Ubuntu-derived distributions. That way we can (and will) set up a separate server to published derived distros, without needing admin intervention for each new distro.

== Proposed fix ==

Replace the script's self.distribution with a list, self.distributions, and loop over it. That gives us the same code path for a single distro (the existing mode of operation) and a loop over several. Next, add code for looking up the derived distros (dead easy since I've done it exactly twice before so it's in a reusable place) and checking that no more than one of the options is given.

== Pre-implementation notes ==

Julian expected me to do this as part of the same work on publish-distro, but as per Sod's Law I ended up forgetting that part. Understandable I hope, since publish-ftpmaster calls publish-distro instead of the other way around.

== Implementation details ==

The self.archives attribute is gone, and self.configs gets one extra layer of indirection. Removing self.archives also let me inline getArchives.

The ordering between setUpDirs and processAccepted has changed. I couldn't see any disadvantage to that, but it let me avoid calling setUpDirs separately for each distro. (This avoids a lot of noise in tests). Another method that stayed outside the distributions loop is recoverWorkingDists; restoring a sane working state of the filesystem now happens right up front for all distributions. The script's lock is tight enough to ensure that these directories aren't moved around later, while the loop is already running.

== Tests ==

{{{
./bin/test -vvc lp.archivepublisher.tests.test_publish_ftpmaster
}}}

== Demo and Q/A ==

Run publish-ftpmaster with the --all-derived option (instead of the usual --distribution=ubuntu). It should publish any derived distros, but not Ubuntu itself.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/publish_ftpmaster.py
  lib/lp/archivepublisher/tests/test_publish_ftpmaster.py

[Henning Eggers (henninge)]

Hello Jeroen,

this looks like it's very well done. Thank you. I just have a few
little suggestion.

 review approve

Cheers,
Henning

> > === modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
> > --- lib/lp/archivepublisher/scripts/publish_ftpmaster.py
> > 2011-08-15 11:04:41 +0000
> > +++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py
> > 2011-08-15 11:04:41 +0000
> > @@ -31,12 +31,20 @@
> > from lp.soyuz.scripts.processaccepted import ProcessAccepted
> > from lp.soyuz.scripts.publishdistro import PublishDistro
> >
> > -# XXX JeroenVermeulen 2011-03-31 bug=746229: to start publishing debug
> > -# archives, get rid of this list.
> > -ARCHIVES_TO_PUBLISH = [
> > - ArchivePurpose.PRIMARY,
> > - ArchivePurpose.PARTNER,
> > - ]
> > +
> > +def get_publishable_archives(distribution):
> > + """Get all archives for `distribution` that should be published."""
> > + # XXX JeroenVermeulen 2011-03-31 bug=746229: to start publishing
> > + # debug archives, simply stop filtering them out here. It may be
> > + # enough to return list(distribution.all_distro_archives) directly.
> > + ARCHIVES_TO_PUBLISH = [
> > + ArchivePurpose.PRIMARY,
> > + ArchivePurpose.PARTNER,
> > + ]
> > + return [
> > + archive
> > + for archive in distribution.all_distro_archives
> > + if archive.purpose in ARCHIVES_TO_PUBLISH]

I don't know if you like lambdas but they are not banned anymore
AFAIK. So this could be written as:

    return filter(
        distribution.all_distro_archives,
        lambda archive: archive.purpose in ARCHIVES_TO_PUBLISH)

But I let you decide what you find more readable. I personally do not
prefer the dangling if clause.

> >
> >
> > def compose_shell_boolean(boolean_value):
> > @@ -166,6 +174,9 @@
> > def add_my_options(self):
> > """See `LaunchpadScript`."""
> > self.parser.add_option(
> > + '-a', '--all-derived', dest='all_derived',
> > action='store_true',
> > + default=False, help="Process all derived distributions.")
> > + self.parser.add_option(
> > '-d', '--distribution', dest='distribution', default=None,
> > help="Distribution to publish.")
> > self.parser.add_option(
> > @@ -179,16 +190,25 @@
> > def processOptions(self):
> > """Handle command-line options.
> >
> > - Sets `self.distribution` to the `Distribution` to publish.
> > + Sets `self.distributions` to the `Distribution`s to publish.
> > """
> > - if self.options.distribution is None:
> > - raise LaunchpadScriptFailure("Specify a distribution.")
> > + if self.options.distribution is None and not
> > self.options.all_derived:
> > + raise LaunchpadScriptFailure(
> > + "Specify a distribution, or --all-derived.")
> > + if self.options.distribution is not None and
> > self.options.all_derived:
> > + raise LaunchpadScriptFailure(
> > + "Can't combine the --distribution and --all-derived
> > options.")
> >
> > - self.distribution = getUtility(IDistributionSet).getByName(
> > - self.options.distribution)
> > - if self.distribution is None:
> > - raise LaunchpadScriptFailure(
> > - "Distribution %s not found." %
> > self.options.distribution)
> > + if self.options.all_derived:
> > + distro_set = getUtility(IDistributionSet)
> > + self.distributions = distro_set.getDe...

review: Approve

[Jeroen T. Vermeulen (jtv)]

> review approve

Thanks again. The notes one by one:

> > > === modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
> > > --- lib/lp/archivepublisher/scripts/publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > +++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > @@ -31,12 +31,20 @@

> > > + return [
> > > + archive
> > > + for archive in distribution.all_distro_archives
> > > + if archive.purpose in ARCHIVES_TO_PUBLISH]
>
> I don't know if you like lambdas but they are not banned anymore
> AFAIK. So this could be written as:
>
> return filter(
> distribution.all_distro_archives,
> lambda archive: archive.purpose in ARCHIVES_TO_PUBLISH)
>
> But I let you decide what you find more readable. I personally do not
> prefer the dangling if clause.

Neither do I, though in this case I feel a very slight preference for the "if" because it reads so naturally.

> > > + self.distributions = distro_set.getDerivedDistributions()
>
> Should the script not say something if this list is empty? Or even
> refuse to start?

Well, there is already a check that a self.distributions has to be selected one way or the other. After that, self.distributions could only be empty if the --all-derived option was passed but there are no derived distributions.

Since this will be running from a cron job, I think we probably shouldn't be generating repeated oopses just because there's no derived distro yet. Or if all derived distros disappear from the database later, I'd like to think that we'd find out about it in other ways. :)

Refusing to start probably wouldn't do us much good: the script would iterate over zero distributions anyway.

> > > === modified file
> > > 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
> > > --- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > +++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
> > > 2011-08-15 11:04:41 +0000
> > > @@ -290,6 +290,22 @@

> > > + reference_config = getPubConfig(distro.main_archive)
> > > + config = script.getConfigs()[distro][ArchivePurpose.PRIMARY]
> > > + self.assertEqual(reference_config.temproot, config.temproot)
> > > + self.assertEqual(reference_config.distroroot,
> > > config.distroroot)
> > > + self.assertEqual(reference_config.archiveroot,
> > > config.archiveroot)
>
> This is a Good Place for the MatchesStructure matcher from testtools.

Yes, it is. Kudos for spotting that it's not in testtools.matchers.__all__ (thus explaining why I didn't find it in __all__). I'll rewrite.

Jeroen