Merge into db-devel : ppa-deletion : Code : Launchpad itself

Reviewer	Review Type	Date Requested	Status
Julian Edwards (community)		2010-03-25	Approve on 2010-04-15
Review via email: mp+22176@code.staging.launchpad.net

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2010-03-25:

#

> === modified file 'lib/lp/archivepublisher/publishing.py'
> --- lib/lp/archivepublisher/publishing.py 2010-02-09 00:17:40 +0000
> +++ lib/lp/archivepublisher/publishing.py 2010-03-25 20:23:14 +0000
> @@ -12,9 +12,11 @@
> import hashlib
> import logging
> import os
> +import shutil
>
> from datetime import datetime
>
> +from storm.store import Store
> from zope.component import getUtility
>
> from lp.archivepublisher import HARDCODED_COMPONENT_ORDER
> @@ -29,13 +31,15 @@
> from canonical.database.sqlbase import sqlvalues
> from lp.registry.interfaces.pocket import (
> PackagePublishingPocket, pocketsuffix)
> -from lp.soyuz.interfaces.archive import ArchivePurpose
> +from lp.soyuz.interfaces.archive import ArchivePurpose, ArchiveStatus
> from lp.soyuz.interfaces.binarypackagerelease import (
> BinaryPackageFormat)
> from lp.soyuz.interfaces.component import IComponentSet
> from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>
> from canonical.librarian.client import LibrarianClient
> +from canonical.launchpad.webapp.errorlog import (
> + ErrorReportingUtility, ScriptRequest)
>
> suffixpocket = dict((v, k) for (k, v) in pocketsuffix.items())
>
> @@ -596,3 +600,37 @@
> in_file.close()
>
> out_file.write(" %s % 16d %s\n" % (checksum, length, file_name))
> +
> + def deleteArchive(self):
> + """Delete the archive.
> +
> + Physically remove the entire archive from disk and set the
archive's
> + status to DELETED.
> +
> + Any errors encountered while removing the archive from disk will
> + be caught and an OOPS report generated.
> + """
> +
> + self.log.info(
> + "Attempting to delete archive '%s/%s' at '%s'." % (
> + self.archive.owner.name, self.archive.name,
> + self._config.archiveroot))
> +
> + # Attempt to rmdir if the path to the root of the archive exists.
> + if os.path.exists(self._config.archiveroot):
> + try:
> + shutil.rmtree(self._config.archiveroot)
> + except:
> + message = 'Exception while deleting archive root %s' % (
> + self._config.archiveroot)
> + request = ScriptRequest([('error-explanation', message)])
> + ErrorReportingUtility().raising(sys.exc_info(), request)
> + self.log.error('%s (%s)' % (message, request.oopsid))
> + else:
> + self.log.warning(
> + "Root directory '%s' for archive '%s/%s' does not exist." %
(
> + self._config.archiveroot, self.archive.owner.name,
> + self.archive.name))
> +
> + # Set archive's status to DELETED.
> + self.archive.status = ArchiveStatus.DELETED

This is all good. I was trying to think of failure scenarios with it but I
think you have them all covered!

>
> === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> --- lib/lp/archivepublisher/tests/test_publisher.py 2010-02-10 00:25:55
+0000
> +++ lib/lp/archivepublisher/tests/test_publisher.py 2010-03-25 20:23:14
+0000
> @@ -26,7 +2...

> === modified file 'lib/lp/archivepublisher/publishing.py'
> --- lib/lp/archivepublisher/publishing.py	2010-02-09 00:17:40 +0000
> +++ lib/lp/archivepublisher/publishing.py	2010-03-25 20:23:14 +0000
> @@ -12,9 +12,11 @@
>  import hashlib
>  import logging
>  import os
> +import shutil
>  
>  from datetime import datetime
>  
> +from storm.store import Store
>  from zope.component import getUtility
>  
>  from lp.archivepublisher import HARDCODED_COMPONENT_ORDER
> @@ -29,13 +31,15 @@
>  from canonical.database.sqlbase import sqlvalues
>  from lp.registry.interfaces.pocket import (
>      PackagePublishingPocket, pocketsuffix)
> -from lp.soyuz.interfaces.archive import ArchivePurpose
> +from lp.soyuz.interfaces.archive import ArchivePurpose, ArchiveStatus
>  from lp.soyuz.interfaces.binarypackagerelease import (
>      BinaryPackageFormat)
>  from lp.soyuz.interfaces.component import IComponentSet
>  from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>  
>  from canonical.librarian.client import LibrarianClient
> +from canonical.launchpad.webapp.errorlog import (
> +    ErrorReportingUtility, ScriptRequest)
>  
>  suffixpocket = dict((v, k) for (k, v) in pocketsuffix.items())
>  
> @@ -596,3 +600,37 @@
>              in_file.close()
>  
>          out_file.write(" %s % 16d %s\n" % (checksum, length, file_name))
> +
> +    def deleteArchive(self):
> +        """Delete the archive.
> +        
> +        Physically remove the entire archive from disk and set the 
archive's 
> +        status to DELETED.
> +
> +        Any errors encountered while removing the archive from disk will 
> +        be caught and an OOPS report generated.
> +        """
> +
> +        self.log.info(
> +            "Attempting to delete archive '%s/%s' at '%s'." % (
> +                self.archive.owner.name, self.archive.name, 
> +                self._config.archiveroot))
> +
> +        # Attempt to rmdir if the path to the root of the archive exists.
> +        if os.path.exists(self._config.archiveroot):
> +            try:
> +                shutil.rmtree(self._config.archiveroot)
> +            except:
> +                message = 'Exception while deleting archive root %s' % (
> +                    self._config.archiveroot)
> +                request = ScriptRequest([('error-explanation', message)])
> +                ErrorReportingUtility().raising(sys.exc_info(), request)
> +                self.log.error('%s (%s)' % (message, request.oopsid))
> +        else:
> +            self.log.warning(
> +                "Root directory '%s' for archive '%s/%s' does not exist." % 
(
> +                    self._config.archiveroot, self.archive.owner.name, 
> +                    self.archive.name))
> +
> +        # Set archive's status to DELETED.
> +        self.archive.status = ArchiveStatus.DELETED

This is all good.  I was trying to think of failure scenarios with it but I
think you have them all covered!

> 
> === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> --- lib/lp/archivepublisher/tests/test_publisher.py	2010-02-10 00:25:55 
+0000
> +++ lib/lp/archivepublisher/tests/test_publisher.py	2010-03-25 20:23:14 
+0000
> @@ -26,7 +26,7 @@
>  from canonical.database.constants import UTC_NOW
>  from canonical.launchpad.ftests.keys_for_tests import gpgkeysdir
>  from lp.soyuz.interfaces.archive import (
> -    ArchivePurpose, IArchiveSet)
> +    ArchivePurpose, ArchiveStatus, IArchiveSet)
>  from lp.soyuz.interfaces.binarypackagerelease import (
>      BinaryPackageFormat)
>  from lp.registry.interfaces.distribution import IDistributionSet
> @@ -95,6 +95,31 @@
>          foo_path = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
>          self.assertEqual(open(foo_path).read().strip(), 'Hello world')
>  
> +    def testDeletingPPA(self):
> +        """Test deleting a PPA"""
> +        ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
> +        test_archive = getUtility(IArchiveSet).new(
> +            distribution=self.ubuntutest, owner=ubuntu_team,
> +            purpose=ArchivePurpose.PPA)
> +
> +        publisher = getPublisher(test_archive, None, self.logger)
> +
> +        pub_source = self.getPubSource(
> +            sourcename="foo", filename="foo_1.dsc",
> +            filecontent='I am a file.',
> +            status=PackagePublishingStatus.PENDING, archive=test_archive)
> +
> +        publisher.A_publish(False)
> +        self.layer.txn.commit()
> +        publisher.C_writeIndexes(False)
> +        publisher.D_writeReleaseFiles(False)
> +        pub_source.sync()

You don't need to do these last 5 lines, nor the getPubSource.

Doing more than necessary makes for a fragile test if other parts of the
system change.  A better test would be to throw some random files into the
root directory of the archive and then assert that they get deleted when
you call deleteArchive, along with the status change.

> +
> +        self.assertTrue(os.path.exists(publisher._config.archiveroot))
> +        publisher.deleteArchive()
> +        self.assertTrue(not os.path.exists(publisher._config.archiveroot))

How about assertFalse?

> +        self.assertEqual(test_archive.status, ArchiveStatus.DELETED)

These are good tests.

> +
>      def testPublishPartner(self):
>          """Test that a partner package is published to the right place."""
>          archive = self.ubuntutest.getArchiveByComponent('partner')
> 
> === modified file 'lib/lp/soyuz/scripts/publishdistro.py'
> --- lib/lp/soyuz/scripts/publishdistro.py	2010-02-24 11:53:22 +0000
> +++ lib/lp/soyuz/scripts/publishdistro.py	2010-03-25 20:23:14 +0000
> @@ -16,7 +16,7 @@
>  from canonical.database.sqlbase import (
>      clear_current_connection_cache, flush_database_updates)
>  from lp.soyuz.interfaces.archive import (
> -    ArchivePurpose, IArchiveSet, MAIN_ARCHIVE_PURPOSES)
> +    ArchivePurpose, ArchiveStatus, IArchiveSet, MAIN_ARCHIVE_PURPOSES)
>  from lp.registry.interfaces.distribution import IDistributionSet
>  from canonical.launchpad.scripts import logger, logger_options
>  from lp.services.scripts.base import LaunchpadScriptFailure
> @@ -191,24 +191,34 @@
>          else:
>              log.info("Processing %s" % archive.archive_url)
>              publisher = getPublisher(archive, allowed_suites, log)
> -
> -        try_and_commit("publishing", publisher.A_publish,
> -                       options.careful or options.careful_publishing)
> -        # Flag dirty pockets for any outstanding deletions.
> -        publisher.A2_markPocketsWithDeletionsDirty()
> -        try_and_commit("dominating", publisher.B_dominate,
> -                       options.careful or options.careful_domination)
> -
> -        # The primary and copy archives use apt-ftparchive to generate the
> -        # indexes, everything else uses the newer internal LP code.
> -        if archive.purpose in (ArchivePurpose.PRIMARY, 
ArchivePurpose.COPY):
> -            try_and_commit("doing apt-ftparchive", 
publisher.C_doFTPArchive,
> -                           options.careful or options.careful_apt)
> +        
> +        # Do we need to delete the archive or publish it?
> +        if archive.status == ArchiveStatus.DELETING:
> +            if archive.purpose == ArchivePurpose.PPA:
> +                # Delete the archive.
> +                try_and_commit("publishing", publisher.deleteArchive,
> +                               options.careful or 
options.careful_publishing)
> +            else:
> +                # Other types of archives do not currently support 
deletion.
> +                pass
>          else:
> -            try_and_commit("building indexes", publisher.C_writeIndexes,
> +            try_and_commit("publishing", publisher.A_publish,
> +                           options.careful or options.careful_publishing)
> +            # Flag dirty pockets for any outstanding deletions.
> +            publisher.A2_markPocketsWithDeletionsDirty()
> +            try_and_commit("dominating", publisher.B_dominate,
> +                           options.careful or options.careful_domination)
> +
> +            # The primary and copy archives use apt-ftparchive to generate 
the
> +            # indexes, everything else uses the newer internal LP code.
> +            if archive.purpose in (ArchivePurpose.PRIMARY, 
ArchivePurpose.COPY):
> +                try_and_commit("doing apt-ftparchive", 
publisher.C_doFTPArchive,
> +                               options.careful or options.careful_apt)
> +            else:
> +                try_and_commit("building indexes", 
publisher.C_writeIndexes,
> +                               options.careful or options.careful_apt)
> +
> +            try_and_commit("doing release files", 
publisher.D_writeReleaseFiles,
>                             options.careful or options.careful_apt)
>  
> -        try_and_commit("doing release files", 
publisher.D_writeReleaseFiles,
> -                       options.careful or options.careful_apt)
> -
>      log.debug("Ciao")

This code has got a subtle bug - it will only process archives marked with
publishing enabled because of the filter just above the context of the diff.

I also think in retrospect it's slightly the wrong approach to take here.
What do you think to adding IArchiveSet.getByStatus()?  Then at the end of
the publisher script we can grab a list of archives that are in the
DELETING state and simply call publisher.deleteArchive().  The new method
on IArchiveSet would require a simple new test.

Otherwise, we're nearly there!  I have a UI branch that's very close to done.

Cheers
J

Revision history for this message

Cody A.W. Somerville (cody-somerville) wrote on 2010-03-26:

#

Download full text (10.1 KiB)

On Thu, Mar 25, 2010 at 5:48 PM, Julian Edwards <
<email address hidden>> wrote:

> >
> > === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> > --- lib/lp/archivepublisher/tests/test_publisher.py 2010-02-10 00:25:55
> +0000
> > +++ lib/lp/archivepublisher/tests/test_publisher.py 2010-03-25 20:23:14
> +0000
> > @@ -26,7 +26,7 @@
> > from canonical.database.constants import UTC_NOW
> > from canonical.launchpad.ftests.keys_for_tests import gpgkeysdir
> > from lp.soyuz.interfaces.archive import (
> > - ArchivePurpose, IArchiveSet)
> > + ArchivePurpose, ArchiveStatus, IArchiveSet)
> > from lp.soyuz.interfaces.binarypackagerelease import (
> > BinaryPackageFormat)
> > from lp.registry.interfaces.distribution import IDistributionSet
> > @@ -95,6 +95,31 @@
> > foo_path = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
> > self.assertEqual(open(foo_path).read().strip(), 'Hello world')
> >
> > + def testDeletingPPA(self):
> > + """Test deleting a PPA"""
> > + ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
> > + test_archive = getUtility(IArchiveSet).new(
> > + distribution=self.ubuntutest, owner=ubuntu_team,
> > + purpose=ArchivePurpose.PPA)
> > +
> > + publisher = getPublisher(test_archive, None, self.logger)
> > +
> > + pub_source = self.getPubSource(
> > + sourcename="foo", filename="foo_1.dsc",
> > + filecontent='I am a file.',
> > + status=PackagePublishingStatus.PENDING,
> archive=test_archive)
> > +
> > + publisher.A_publish(False)
> > + self.layer.txn.commit()
> > + publisher.C_writeIndexes(False)
> > + publisher.D_writeReleaseFiles(False)
> > + pub_source.sync()
>
> You don't need to do these last 5 lines, nor the getPubSource.
>
> Doing more than necessary makes for a fragile test if other parts of the
> system change. A better test would be to throw some random files into the
> root directory of the archive and then assert that they get deleted when
> you call deleteArchive, along with the status change.
>

You bring up a good point. However, I think what we need to do to
successfully test our ability to delete an archive and not just our ability
to delete a directory with some files in it is to attempt deleting a simple
but typical archive. If there is higher level way of achieving the same
result instead of calling those publisher methods manually then I think that
would be a good compromise. What do you think?

> > +
> > + self.assertTrue(os.path.exists(publisher._config.archiveroot))
> > + publisher.deleteArchive()
> > + self.assertTrue(not
> os.path.exists(publisher._config.archiveroot))
>
> How about assertFalse?
>

ACK.

<snip>

> === modified file 'lib/lp/soyuz/scripts/publishdistro.py'
> > --- lib/lp/soyuz/scripts/publishdistro.py 2010-02-24 11:53:22 +0000
> > +++ lib/lp/soyuz/scripts/publishdistro.py 2010-03-25 20:23:14 +0000
> > @@ -16,7 +16,7 @@
> > from canonical.database.sqlbase import (
> > clear_current_connection_cache, flush_database_updates)
> > from lp.soyuz.interfaces.archive import (
>...

On Thu, Mar 25, 2010 at 5:48 PM, Julian Edwards <
julian.edwards@canonical.com> wrote:

> >
> > === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> > --- lib/lp/archivepublisher/tests/test_publisher.py   2010-02-10 00:25:55
> +0000
> > +++ lib/lp/archivepublisher/tests/test_publisher.py   2010-03-25 20:23:14
> +0000
> > @@ -26,7 +26,7 @@
> >  from canonical.database.constants import UTC_NOW
> >  from canonical.launchpad.ftests.keys_for_tests import gpgkeysdir
> >  from lp.soyuz.interfaces.archive import (
> > -    ArchivePurpose, IArchiveSet)
> > +    ArchivePurpose, ArchiveStatus, IArchiveSet)
> >  from lp.soyuz.interfaces.binarypackagerelease import (
> >      BinaryPackageFormat)
> >  from lp.registry.interfaces.distribution import IDistributionSet
> > @@ -95,6 +95,31 @@
> >          foo_path = "%s/main/f/foo/foo_666.dsc" % self.pool_dir
> >          self.assertEqual(open(foo_path).read().strip(), 'Hello world')
> >
> > +    def testDeletingPPA(self):
> > +        """Test deleting a PPA"""
> > +        ubuntu_team = getUtility(IPersonSet).getByName('ubuntu-team')
> > +        test_archive = getUtility(IArchiveSet).new(
> > +            distribution=self.ubuntutest, owner=ubuntu_team,
> > +            purpose=ArchivePurpose.PPA)
> > +
> > +        publisher = getPublisher(test_archive, None, self.logger)
> > +
> > +        pub_source = self.getPubSource(
> > +            sourcename="foo", filename="foo_1.dsc",
> > +            filecontent='I am a file.',
> > +            status=PackagePublishingStatus.PENDING,
> archive=test_archive)
> > +
> > +        publisher.A_publish(False)
> > +        self.layer.txn.commit()
> > +        publisher.C_writeIndexes(False)
> > +        publisher.D_writeReleaseFiles(False)
> > +        pub_source.sync()
>
> You don't need to do these last 5 lines, nor the getPubSource.
>
> Doing more than necessary makes for a fragile test if other parts of the
> system change.  A better test would be to throw some random files into the
> root directory of the archive and then assert that they get deleted when
> you call deleteArchive, along with the status change.
>

You bring up a good point. However, I think what we need to do to
successfully test our ability to delete an archive and not just our ability
to delete a directory with some files in it is to attempt deleting a simple
but typical archive. If there is higher level way of achieving the same
result instead of calling those publisher methods manually then I think that
would be a good compromise. What do you think?

> > +
> > +        self.assertTrue(os.path.exists(publisher._config.archiveroot))
> > +        publisher.deleteArchive()
> > +        self.assertTrue(not
> os.path.exists(publisher._config.archiveroot))
>
> How about assertFalse?
>

ACK.

<snip>

> === modified file 'lib/lp/soyuz/scripts/publishdistro.py'
> > --- lib/lp/soyuz/scripts/publishdistro.py     2010-02-24 11:53:22 +0000
> > +++ lib/lp/soyuz/scripts/publishdistro.py     2010-03-25 20:23:14 +0000
> > @@ -16,7 +16,7 @@
> >  from canonical.database.sqlbase import (
> >      clear_current_connection_cache, flush_database_updates)
> >  from lp.soyuz.interfaces.archive import (
> > -    ArchivePurpose, IArchiveSet, MAIN_ARCHIVE_PURPOSES)
> > +    ArchivePurpose, ArchiveStatus, IArchiveSet, MAIN_ARCHIVE_PURPOSES)
> >  from lp.registry.interfaces.distribution import IDistributionSet
> >  from canonical.launchpad.scripts import logger, logger_options
> >  from lp.services.scripts.base import LaunchpadScriptFailure
> > @@ -191,24 +191,34 @@
> >          else:
> >              log.info("Processing %s" % archive.archive_url)
> >              publisher = getPublisher(archive, allowed_suites, log)
> > -
> > -        try_and_commit("publishing", publisher.A_publish,
> > -                       options.careful or options.careful_publishing)
> > -        # Flag dirty pockets for any outstanding deletions.
> > -        publisher.A2_markPocketsWithDeletionsDirty()
> > -        try_and_commit("dominating", publisher.B_dominate,
> > -                       options.careful or options.careful_domination)
> > -
> > -        # The primary and copy archives use apt-ftparchive to generate
> the
> > -        # indexes, everything else uses the newer internal LP code.
> > -        if archive.purpose in (ArchivePurpose.PRIMARY,
> ArchivePurpose.COPY):
> > -            try_and_commit("doing apt-ftparchive",
> publisher.C_doFTPArchive,
> > -                           options.careful or options.careful_apt)
> > +
> > +        # Do we need to delete the archive or publish it?
> > +        if archive.status == ArchiveStatus.DELETING:
> > +            if archive.purpose == ArchivePurpose.PPA:
> > +                # Delete the archive.
> > +                try_and_commit("publishing", publisher.deleteArchive,
> > +                               options.careful or
> options.careful_publishing)
> > +            else:
> > +                # Other types of archives do not currently support
> deletion.
> > +                pass
> >          else:
> > -            try_and_commit("building indexes", publisher.C_writeIndexes,
> > +            try_and_commit("publishing", publisher.A_publish,
> > +                           options.careful or
> options.careful_publishing)
> > +            # Flag dirty pockets for any outstanding deletions.
> > +            publisher.A2_markPocketsWithDeletionsDirty()
> > +            try_and_commit("dominating", publisher.B_dominate,
> > +                           options.careful or
> options.careful_domination)
> > +
> > +            # The primary and copy archives use apt-ftparchive to
> generate
> the
> > +            # indexes, everything else uses the newer internal LP code.
> > +            if archive.purpose in (ArchivePurpose.PRIMARY,
> ArchivePurpose.COPY):
> > +                try_and_commit("doing apt-ftparchive",
> publisher.C_doFTPArchive,
> > +                               options.careful or options.careful_apt)
> > +            else:
> > +                try_and_commit("building indexes",
> publisher.C_writeIndexes,
> > +                               options.careful or options.careful_apt)
> > +
> > +            try_and_commit("doing release files",
> publisher.D_writeReleaseFiles,
> >                             options.careful or options.careful_apt)
> >
> > -        try_and_commit("doing release files",
> publisher.D_writeReleaseFiles,
> > -                       options.careful or options.careful_apt)
> > -
> >      log.debug("Ciao")
>
> This code has got a subtle bug - it will only process archives marked with
> publishing enabled because of the filter just above the context of the
> diff.
>

This was actually an intentional decision. Deleting an archive is a function
of the publisher. If the publisher is disabled for an archive, I don't think
we should process requests to delete the archive just like we wouldn't
process pending publications or requests to copy a package into the archive.

You're welcome to disagree on me this. I suspect that you might reply with
that the flag's actual intention is to 'disable publishing for the archive',
not 'disable the publisher for that archive'. However, I think even if this
is the case then we should consider re-purposing it to include this slightly
broader scope of functionality. If you're interested in my rationale or some
potential use cases for this, I'd be happy to discuss it further with you on
IRC.

However, looking at your UI branch, it seems that you disable the PPA as
well as set the status to ArchiveStatus.DELETING which of course puts a
'wrench' in this plan. See below for further thoughts related to this.

I also think in retrospect it's slightly the wrong approach to take here.
> What do you think to adding IArchiveSet.getByStatus()?  Then at the end of
> the publisher script we can grab a list of archives that are in the
> DELETING state and simply call publisher.deleteArchive().  The new method
> on IArchiveSet would require a simple new test.
>

Could you elaborate on what exactly you're referring to? I think there is a
lot that could be improved in this script (its a bit of a hack!).

However, you've made me notice a glaring bug. Once an archive's status is
set to ArchiveStatus.DELETED it'll start getting published again. However,
because PPAs are filtered by those with pending publications it means that
deleted PPAs won't get published unless it somehow gets a pending
publication (which we don't necessarily do anything to actively prevent and
I wouldn't trust that such a scenario wouldn't somehow occur).

I think it may make sense to take the time and refractor how we get the
archives we want the publisher to consider (this might be a good opportunity
to move some of the soyuz methods out of the registry Distribution model,
normalize them, and move them into IArchiveSet). The end result that I think
this could provide us with is easier reuse and testing of the logic used to
determine which archives needed to be considered by the publisher. For
example, how do you test that the publisher won't publish archives that have
publishing disabled or that are deleted?

Lets look at all the different ways we get the archive(s) we want to publish
depending on the mode of the script:

1. [distribution.getArchiveByComponent('partner')]
2. distribution.getAllPPAs()
3. distribution.getPendingPublicationPPAs()
4. getUtility(IArchiveSet).getByDistroPurpose(
            distribution, ArchivePurpose.DEBUG)
5. getUtility(IArchiveSet).getArchivesForDistribution(
            distribution, purposes=[ArchivePurpose.COPY])
6. [distribution.main_archive]

Do we really want to add to this list? Besides, do we really want to slap on
yet another method to IArchiveSet - maybe yes, maybe no but I don't think so
for the former.

Also, should this script be responsible for determining what the publisher
should do with an archive? In my opinion, I think it might make sense for
that responsibility to belong to the publisher and for this script to be
responsible for configuring the publisher based on the settings passed to
the script and passing the set of archives that we want it to consider.

Otherwise, we're nearly there!  I have a UI branch that's very close to
> done.
>

Woot! :) We are indeed.

Cheers,

Cody

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2010-03-26:

#

Download full text (6.5 KiB)

On Friday 26 March 2010 01:15:27 Cody A.W. Somerville wrote:
> > Doing more than necessary makes for a fragile test if other parts of the
> > system change. A better test would be to throw some random files into
> > the root directory of the archive and then assert that they get deleted
> > when you call deleteArchive, along with the status change.
>
> You bring up a good point. However, I think what we need to do to
> successfully test our ability to delete an archive and not just our ability
> to delete a directory with some files in it is to attempt deleting a simple
> but typical archive. If there is higher level way of achieving the same
> result instead of calling those publisher methods manually then I think
> that would be a good compromise. What do you think?

In the nicest possible way, I think it's crazy.

The code you're testing doesn't care what you're deleting, it just does a
shutil.rmtree(). It's totally pointless putting any kind of data structure in
there because you're not using it; it just wastes test suite time. If we were
testing deleting of only certain parts of the archive, then of course it would
be correct to set up a real archive. But we're not.

Seriously, just poke some files in the archive root and test that they get
removed.

> > This code has got a subtle bug - it will only process archives marked
> > with publishing enabled because of the filter just above the context of
> > the diff.
>
> This was actually an intentional decision. Deleting an archive is a
> function of the publisher. If the publisher is disabled for an archive, I
> don't think we should process requests to delete the archive just like we
> wouldn't process pending publications or requests to copy a package into
> the archive.
>
> You're welcome to disagree on me this. I suspect that you might reply with
> that the flag's actual intention is to 'disable publishing for the
> archive', not 'disable the publisher for that archive'. However, I think

Correct :)

> even if this is the case then we should consider re-purposing it to
> include this slightly broader scope of functionality. If you're interested
> in my rationale or some potential use cases for this, I'd be happy to
> discuss it further with you on IRC.
>
> However, looking at your UI branch, it seems that you disable the PPA as
> well as set the status to ArchiveStatus.DELETING which of course puts a
> 'wrench' in this plan. See below for further thoughts related to this.

Think about this from the user's point of view (and the user's PoV should
drive everything). They're clicking a button that says to delete the PPA.
This is an action that has a certain amount of finality about it! Having to
then go and discover that it didn't really delete because you forgot to tick a
"publish" checkbox on the +edit page is rather weird, don't you think?

> > What do you think to adding IArchiveSet.getByStatus()? Then at the end
> > of the publisher script we can grab a list of archives that are in the
> > DELETING state and simply call publisher.deleteArchive(). The new method
> > on IArchiveSet would require a simple new test.
>
> Could you elaborate on what exactly y...

On Friday 26 March 2010 01:15:27 Cody A.W. Somerville wrote:
> > Doing more than necessary makes for a fragile test if other parts of the
> > system change.  A better test would be to throw some random files into
> > the root directory of the archive and then assert that they get deleted
> > when you call deleteArchive, along with the status change.
> 
> You bring up a good point. However, I think what we need to do to
> successfully test our ability to delete an archive and not just our ability
> to delete a directory with some files in it is to attempt deleting a simple
> but typical archive. If there is higher level way of achieving the same
> result instead of calling those publisher methods manually then I think
>  that would be a good compromise. What do you think?

In the nicest possible way, I think it's crazy.

The code you're testing doesn't care what you're deleting, it just does a 
shutil.rmtree().  It's totally pointless putting any kind of data structure in 
there because you're not using it; it just wastes test suite time.  If we were 
testing deleting of only certain parts of the archive, then of course it would 
be correct to set up a real archive.  But we're not.

Seriously, just poke some files in the archive root and test that they get 
removed.

> > This code has got a subtle bug - it will only process archives marked
> > with publishing enabled because of the filter just above the context of
> > the diff.
> 
> This was actually an intentional decision. Deleting an archive is a
>  function of the publisher. If the publisher is disabled for an archive, I
>  don't think we should process requests to delete the archive just like we
>  wouldn't process pending publications or requests to copy a package into
>  the archive.
> 
> You're welcome to disagree on me this. I suspect that you might reply with
> that the flag's actual intention is to 'disable publishing for the
>  archive', not 'disable the publisher for that archive'. However, I think

Correct :)

>  even if this is the case then we should consider re-purposing it to
>  include this slightly broader scope of functionality. If you're interested
>  in my rationale or some potential use cases for this, I'd be happy to
>  discuss it further with you on IRC.
> 
> However, looking at your UI branch, it seems that you disable the PPA as
> well as set the status to ArchiveStatus.DELETING which of course puts a
> 'wrench' in this plan. See below for further thoughts related to this.

Think about this from the user's point of view (and the user's PoV should 
drive everything).  They're clicking a button that says to delete the PPA.  
This is an action that has a certain amount of finality about it!  Having to 
then go and discover that it didn't really delete because you forgot to tick a 
"publish" checkbox on the +edit page is rather weird, don't you think?

> > What do you think to adding IArchiveSet.getByStatus()?  Then at the end
> > of the publisher script we can grab a list of archives that are in the
> > DELETING state and simply call publisher.deleteArchive().  The new method
> > on IArchiveSet would require a simple new test.
> 
> Could you elaborate on what exactly you're referring to? I think there is a
> lot that could be improved in this script (its a bit of a hack!).

The script is gross, but I don't want to make wholesale improvements to it in 
this branch, let's keep focused (although drive-by fixes are great and 
encouraged).

> However, you've made me notice a glaring bug. Once an archive's status is
> set to ArchiveStatus.DELETED it'll start getting published again. However,
> because PPAs are filtered by those with pending publications it means that
> deleted PPAs won't get published unless it somehow gets a pending
> publication (which we don't necessarily do anything to actively prevent and
> I wouldn't trust that such a scenario wouldn't somehow occur).

Actually we do - disabling the PPA prevents uploads and copying.

However, as well as setting DELETED, you should set publish=False to be on the 
safe side.  This will also act as a cleanup marker when we fold these booleans 
into the status enum.

> I think it may make sense to take the time and refractor how we get the
> archives we want the publisher to consider (this might be a good
>  opportunity to move some of the soyuz methods out of the registry
>  Distribution model, normalize them, and move them into IArchiveSet). The
>  end result that I think this could provide us with is easier reuse and
>  testing of the logic used to determine which archives needed to be
>  considered by the publisher.

+1 but not in this branch.  Let's redesign Soyuz one step at a time and keep 
the branches limited in focus.  You'll appreciate why if you ever want to get 
a diff of a revno that fixed a bug.

Also, I really want to get this backend landed before PQM closes at 2200 
*today*, then we can let the UI changes bake on edge next cycle.

>  For example, how do you test that the
>  publisher won't publish archives that have publishing disabled or that are
>  deleted?

Yes, this is an architectural problem with that script that needs fixing.

> Lets look at all the different ways we get the archive(s) we want to
>  publish depending on the mode of the script:
> 
> 1. [distribution.getArchiveByComponent('partner')]
> 2. distribution.getAllPPAs()
> 3. distribution.getPendingPublicationPPAs()
> 4. getUtility(IArchiveSet).getByDistroPurpose(
>             distribution, ArchivePurpose.DEBUG)
> 5. getUtility(IArchiveSet).getArchivesForDistribution(
>             distribution, purposes=[ArchivePurpose.COPY])
> 6. [distribution.main_archive]

Hmm #4 and #5 look suspiciously similar!

> Do we really want to add to this list? Besides, do we really want to slap
>  on yet another method to IArchiveSet - maybe yes, maybe no but I don't
>  think so for the former.

I don't see any problem adding to this list.  The method I suggested is also 
going to be very useful in the future as we add more statuses, which is why I 
suggested it.

> Also, should this script be responsible for determining what the publisher
> should do with an archive? In my opinion, I think it might make sense for
> that responsibility to belong to the publisher and for this script to be
> responsible for configuring the publisher based on the settings passed to
> the script and passing the set of archives that we want it to consider.

Neither!  It should be a property/behaviour of the archive.  But like I said, 
let's tackle one thing at a time.

Thanks for the great input though, your suggestions will be very useful.

Let's make the fixes as I suggest and we can land this puppy.

Cheers
J

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2010-03-26:

#

Oh one more thing:

lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py

This needs changing so it ignores publish=False and status=DELETED otherwise I expect it'll blow up when the directory gets removed.

Revision history for this message

Cody A.W. Somerville (cody-somerville) wrote on 2010-04-15:

#

Download full text (4.8 KiB)

> On Friday 26 March 2010 01:15:27 Cody A.W. Somerville wrote:
> > > Doing more than necessary makes for a fragile test if other parts of the
> > > system change. A better test would be to throw some random files into
> > > the root directory of the archive and then assert that they get deleted
> > > when you call deleteArchive, along with the status change.
> >
> > You bring up a good point. However, I think what we need to do to
> > successfully test our ability to delete an archive and not just our ability
> > to delete a directory with some files in it is to attempt deleting a simple
> > but typical archive. If there is higher level way of achieving the same
> > result instead of calling those publisher methods manually then I think
> > that would be a good compromise. What do you think?
>
> In the nicest possible way, I think it's crazy.
>
> The code you're testing doesn't care what you're deleting, it just does a
> shutil.rmtree(). It's totally pointless putting any kind of data structure in
> there because you're not using it; it just wastes test suite time. If we were
> testing deleting of only certain parts of the archive, then of course it would
> be correct to set up a real archive. But we're not.
>
> Seriously, just poke some files in the archive root and test that they get
> removed.

Okay.

> > > This code has got a subtle bug - it will only process archives marked
> > > with publishing enabled because of the filter just above the context of
> > > the diff.
> >
> > This was actually an intentional decision. Deleting an archive is a
> > function of the publisher. If the publisher is disabled for an archive, I
> > don't think we should process requests to delete the archive just like we
> > wouldn't process pending publications or requests to copy a package into
> > the archive.
> >
> > You're welcome to disagree on me this. I suspect that you might reply with
> > that the flag's actual intention is to 'disable publishing for the
> > archive', not 'disable the publisher for that archive'. However, I think
>
> Correct :)
>
> > even if this is the case then we should consider re-purposing it to
> > include this slightly broader scope of functionality. If you're interested
> > in my rationale or some potential use cases for this, I'd be happy to
> > discuss it further with you on IRC.
> >
> > However, looking at your UI branch, it seems that you disable the PPA as
> > well as set the status to ArchiveStatus.DELETING which of course puts a
> > 'wrench' in this plan. See below for further thoughts related to this.
>
> Think about this from the user's point of view (and the user's PoV should
> drive everything). They're clicking a button that says to delete the PPA.
> This is an action that has a certain amount of finality about it! Having to
> then go and discover that it didn't really delete because you forgot to tick a
> "publish" checkbox on the +edit page is rather weird, don't you think?

I don't feel strongly about this. However, the publish bool is not exposed to users (or on the form at all for anyone IIRC) so they'd only run into this if an admin intentionally disabled publishing for an archive. This I wo...

Launchpad itself

Merge lp://staging/~cody-somerville/launchpad/ppa-deletion into lp://staging/launchpad/db-devel

Commit message

Description of the change

Preview Diff

Subscribers