Code review comment for lp:~jtv/launchpad/bug-824499

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.getDerivedDistributions()

Should the script not say something if this list is empty? Or even
refuse to start?

> > + else:
> > + distro = getUtility(IDistributionSet).getByName(
> > + self.options.distribution)
> > + if distro is None:
> > + raise LaunchpadScriptFailure(
> > + "Distribution %s not found." %
> > self.options.distribution)
> > + self.distributions = [distro]
> >
> > def executeShell(self, command_line, failure=None):
> > """Run `command_line` through a shell.
[...]

> > === 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 @@
> > self.SCRIPT_PATH + " -d ubuntu")
> > self.assertEqual(0, retval, "Script failure:\n" + stderr)
> >
> > + def
> > test_getConfigs_maps_distro_and_purpose_to_matching_config(self):
> > + distro = self.makeDistroWithPublishDirectory()
> > + script = self.makeScript(distro)
> > + script.setUp()
> > + 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.