launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/_schema_circular_imports.py'
> --- lib/lp/_schema_circular_imports.py	2016-10-12 23:24:24 +0000
> +++ lib/lp/_schema_circular_imports.py	2016-11-07 17:02:58 +0000
> @@ -484,6 +484,10 @@
>      IArchive, '_addArchiveDependency', 'pocket', PackagePublishingPocket)
>  patch_entry_return_type(
>      IArchive, '_addArchiveDependency', IArchiveDependency)
> +patch_plain_parameter_type(
> +    IArchive, 'markSuiteDirty', 'distroseries', IDistroSeries)
> +patch_choice_parameter_type(
> +    IArchive, 'markSuiteDirty', 'pocket', PackagePublishingPocket)

I'm not entirely sure how PackagePublishingPocket can have circular import issues nowadays -- lp.registry.interfaces.pocket only imports lazr.enum.

IDistroSeries should nearly not either, but I can believe that one.

>  
>  # IBuildFarmJob
>  patch_choice_property(IBuildFarmJob, 'status', BuildStatus)
> 
> === modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
> --- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2016-01-13 17:54:05 +0000
> +++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2016-11-07 17:02:58 +0000
> @@ -631,9 +623,8 @@
>              if security_only:
>                  has_published = self.publishSecurityUploads(distribution)
>              else:
> -                updated_suites = self.updateStagedFiles(distribution)
> -                self.publishDistroUploads(
> -                    distribution, updated_suites=updated_suites)
> +                self.updateStagedFiles(distribution)

We would do well to commit here (or possibly right after the markSuiteDirty); we don't really want to be holding a long FOR NO KEY UPDATE on archive 1.

> +                self.publishDistroUploads(distribution)
>                  # Let's assume the main archive is always modified
>                  has_published = True
>  
> 
> === modified file 'lib/lp/archivepublisher/scripts/publishdistro.py'
> --- lib/lp/archivepublisher/scripts/publishdistro.py	2016-09-19 10:28:33 +0000
> +++ lib/lp/archivepublisher/scripts/publishdistro.py	2016-11-07 17:02:58 +0000
> @@ -235,6 +235,19 @@
>              suites.add((series.name, pocket))
>          return suites
>  
> +    def findDirtySuites(self, distribution, archive):

findExplicitlyDirtySuites, maybe? Dirty suites won't be returned by this method 99.9% of the time.

Also, distribution is implied by archive, and we should be moving away from the publisher ever treating distribution as a top-level object.

> +        """Find the suites that have been explicitly marked as dirty."""
> +        for suite in self.options.dirty_suites:
> +            yield self.findSuite(distribution, suite)
> +        if archive.dirty_suites is not None:
> +            for suite in archive.dirty_suites:
> +                try:
> +                    yield distribution.getDistroSeriesAndPocket(suite)
> +                except NotFoundError:
> +                    self.logger.exception(
> +                        "Failed to parse dirty suite '%s' for archive '%s'" %
> +                        (suite, archive.reference))
> +
>      def getCopyArchives(self, distribution):
>          """Find copy archives for the selected distribution."""
>          copy_archives = list(
> @@ -347,14 +360,14 @@
>                      elif archive.can_be_published:
>                          publisher = self.getPublisher(
>                              distribution, archive, allowed_suites)
> -                        for suite in self.options.dirty_suites:
> -                            distroseries, pocket = self.findSuite(
> -                                distribution, suite)
> +                        for distroseries, pocket in self.findDirtySuites(
> +                                distribution, archive):
>                              if not cannot_modify_suite(
>                                      archive, distroseries, pocket):
>                                  publisher.markPocketDirty(
>                                      distroseries, pocket)
>                          self.publishArchive(archive, publisher)
> +                        archive.dirty_suites = None

Since publishArchive commits often, this has the potentially entertaining effect of forgetting any explicit dirt that was added while the publisher was running -- most of the time, for the Ubuntu primary archive.

>                          work_done = True
>                      else:
>                          work_done = False
> 
> === modified file 'lib/lp/registry/model/distribution.py'
> --- lib/lp/registry/model/distribution.py	2016-10-03 13:10:11 +0000
> +++ lib/lp/registry/model/distribution.py	2016-11-07 17:02:58 +0000
> @@ -1296,13 +1296,23 @@
>              reapable_af_query, clauseTables=['ArchiveFile'],
>              orderBy=['archive.id'], distinct=True)
>  
> +        dirty_suites_query = """
> +        Archive.purpose = %s AND
> +        Archive.distribution = %s AND
> +        Archive.dirty_suites IS NOT NULL
> +        """ % sqlvalues(ArchivePurpose.PPA, self)
> +
> +        dirty_suites_archives = Archive.select(
> +            dirty_suites_query, orderBy=['archive.id'], distinct=True)

Gratuitous use of distinct.

> +
>          deleting_archives = Archive.selectBy(
>              distribution=self,
>              purpose=ArchivePurpose.PPA,
>              status=ArchiveStatus.DELETING).orderBy(['archive.id'])
>  
>          return src_archives.union(bin_archives).union(
> -            reapable_af_archives).union(deleting_archives)
> +            reapable_af_archives).union(dirty_suites_archives).union(
> +            deleting_archives)
>  
>      def getArchiveByComponent(self, component_name):
>          """See `IDistribution`."""
> 
> === modified file 'lib/lp/soyuz/configure.zcml'
> --- lib/lp/soyuz/configure.zcml	2016-09-24 06:22:43 +0000
> +++ lib/lp/soyuz/configure.zcml	2016-11-07 17:02:58 +0000
> @@ -327,8 +327,8 @@
>          <require
>              permission="launchpad.Edit"
>              interface="lp.soyuz.interfaces.archive.IArchiveEdit"
> -            set_attributes="build_debug_symbols description displayname
> -                            publish publish_debug_symbols status
> +            set_attributes="build_debug_symbols description dirty_suites
> +                            displayname publish publish_debug_symbols status
>                              suppress_subscription_notifications"/>

Should this be on InternalScriptsOnly? markSuiteDirty's pokes don't go through the security proxy, so it's just the publisher's = None that needs to be permitted.

>          <!--
>             NOTE: The 'private' permission controls who can turn a public
> 
> === modified file 'lib/lp/soyuz/model/archive.py'
> --- lib/lp/soyuz/model/archive.py	2016-10-20 11:49:45 +0000
> +++ lib/lp/soyuz/model/archive.py	2016-11-07 17:02:58 +0000
> @@ -2414,6 +2417,14 @@
>          Store.of(pcj.context).remove(pcj.context)
>          job.destroySelf()
>  
> +    def markSuiteDirty(self, distroseries, pocket):
> +        """See `IArchive`."""
> +        suite = distroseries.getSuite(pocket)

Should probably verify that the distroseries is relevant to this archive.

> +        if self.dirty_suites is None:
> +            self.dirty_suites = [suite]
> +        elif suite not in self.dirty_suites:
> +            self.dirty_suites.append(suite)
> +
>  
>  def validate_ppa(owner, distribution, proposed_name, private=False):
>      """Can 'person' create a PPA called 'proposed_name'?


-- 
https://code.launchpad.net/~cjwatson/launchpad/archive-api-dirty/+merge/310208
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.