Merge lp:~wgrant/launchpad/no-lucilleconfig-directories into lp:launchpad

[William Grant]

[r=stevenk][ui=none][bug=691784] Calculate archive publication paths from the LAZR config, not lucilleconfig.

A continuation of my anti-lucilleconfig tirade from lp:~wgrant/launchpad/destroy-distroseries-lucilleconfig, this branch obsoletes Distribution.lucilleconfig's "publishing" section.

getPubConfig has conditionally overridden Config's paths since PPAs were introduced some years ago. This branch moves the calculation out of Config and into getPubConfig, replacing dependency on in-database configuration with values that are already in the LAZR config.

Before this can land, production configs need to be checked. The only change should be that ${archivepublisher.root}/ubuntu-test is now used instead of /srv/launchpad.net/ubuntu-archive/ubuntu-test, but this may make germanium unhappy.

[Steve Kowalik (stevenk)]

Hi William,

Firstly, thank you for the clean up. My two concerns are:

 * Why are you adding another variable APT_FTPARCHIVE_PURPOSES? It's there one that already encompasses that from lp.soyuz.interfaces.archive?
 * Do any other callsites utilitise the code you've changed and therefore their tests need fixing too?

review: Needs Information (code*)

[Steve Kowalik (stevenk)]

I was mistaken, there isn't a variable that encompasses PRIMARY and DEBUG in lp.soyuz.interfaces.archive. I think there should be, since there is likely other places the code that could benefit from it.

[Steve Kowalik (stevenk)]

After discussions with William, my first objection is withdrawn, your honour.

review: Approve (code*)

[Tim Penhey (thumper)]

review: Approve (mentor)

=== modified file 'lib/lp/archivepublisher/config.py'
--- lib/lp/archivepublisher/config.py	2010-10-15 08:06:41 +0000
+++ lib/lp/archivepublisher/config.py	2010-12-18 00:21:49 +0000
@@ -14,16 +14,7 @@
 from lp.soyuz.enums import ArchivePurpose
 
 
-def update_pub_config(pubconf):
-    """Update dependent `PubConfig` fields.
-
-    Update fields dependending on 'archiveroot'.
-    """
-    pubconf.poolroot = os.path.join(pubconf.archiveroot, 'pool')
-    pubconf.distsroot = os.path.join(pubconf.archiveroot, 'dists')
-    pubconf.overrideroot = None
-    pubconf.cacheroot = None
-    pubconf.miscroot = None
+APT_FTPARCHIVE_PURPOSES = (ArchivePurpose.PRIMARY, ArchivePurpose.COPY)
 
 
 def getPubConfig(archive):
@@ -36,9 +27,10 @@
     pubconf = Config(archive.distribution)
     ppa_config = config.personalpackagearchive
 
-    if archive.purpose == ArchivePurpose.PRIMARY:
-        pass
-    elif archive.is_ppa:
+    pubconf.temproot = os.path.join(
+        config.archivepublisher.root, '%s-temp' % pubconf.distroName)
+
+    if archive.is_ppa:
         if archive.private:
             pubconf.distroroot = ppa_config.private_root
             pubconf.htaccessroot = os.path.join(
@@ -49,34 +41,41 @@
         pubconf.archiveroot = os.path.join(
             pubconf.distroroot, archive.owner.name, archive.name,
             archive.distribution.name)
-        update_pub_config(pubconf)
-    elif archive.purpose == ArchivePurpose.PARTNER:
-        pubconf.distroroot = config.archivepublisher.root
-        pubconf.archiveroot = os.path.join(
-            pubconf.distroroot, archive.distribution.name + '-partner')
-        update_pub_config(pubconf)
-    elif archive.purpose == ArchivePurpose.DEBUG:
-        pubconf.distroroot = config.archivepublisher.root
-        pubconf.archiveroot = os.path.join(
-            pubconf.distroroot, archive.distribution.name + '-debug')
-        update_pub_config(pubconf)
+    elif archive.is_main:
+        pubconf.distroroot = config.archivepublisher.root
+        pubconf.archiveroot = os.path.join(
+            pubconf.distroroot, archive.distribution.name)
+        if archive.purpose == ArchivePurpose.PARTNER:
+            pubconf.archiveroot += '-partner'
+        elif archive.purpose == ArchivePurpose.DEBUG:
+            pubconf.archiveroot += '-debug'
     elif archive.is_copy:
         pubconf.distroroot = config.archivepublisher.root
         pubconf.archiveroot = os.path.join(
             pubconf.distroroot,
             archive.distribution.name + '-' + archive.name,
             archive.distribution.name)
-        # Multiple copy archives can exist on the same machine so the
-        # temp areas need to be unique also.
+    else:
+        raise AssertionError(
+            "Unknown archive purpose %s when getting publisher config.",
+            archive.purpose)
+
+    # There can be multiple copy archives, so the temp dir needs to be
+    # within the archive.
+    if archive.is_copy:
         pubconf.temproot = pubconf.archiveroot + '-temp'
-        update_pub_config(pubconf)
+
+    if archive.purpose in APT_FTPARCHIVE_PURPOSES:
         pubconf.overrideroot = pubconf.archiveroot + '-overrides'
         pubconf.cacheroot = pubconf.archiveroot + '-cache'
         pubconf.miscroot = pubconf.archiveroot + '-misc'
     else:
-        raise AssertionError(
-            "Unknown archive purpose %s when getting publisher config.",
-            archive.purpose)
+        pubconf.overrideroot = None
+        pubconf.cacheroot = None
+        pubconf.miscroot = None
+
+    pubconf.poolroot = os.path.join(pubconf.archiveroot, 'pool')
+    pubconf.distsroot = os.path.join(pubconf.archiveroot, 'dists')
 
     meta_root = os.path.join(
         pubconf.distroroot, archive.owner.name)
@@ -140,20 +139,6 @@
         self.stayofexecution = self._distroconfig.get(
             "publishing", "pendingremovalduration", 5)
         self.stayofexecution = float(self.stayofexecution)
-        self.distroroot = self._distroconfig.get("publishing","root")
-        self.archiveroot = self._distroconfig.get("publishing","archiveroot")
-        self.poolroot = self._distroconfig.get("publishing","poolroot")
-        self.distsroot = self._distroconfig.get("publishing","distsroot")
-        self.overrideroot = self._distroconfig.get(
-            "publishing","overrideroot")
-        self.cacheroot = self._distroconfig.get("publishing","cacheroot")
-        self.miscroot = self._distroconfig.get("publishing","miscroot")
-        # XXX cprov 2007-04-26 bug=45270:
-        # We should build all the previous attributes
-        # dynamically like this. It would reduce the configuration complexity.
-        # Even before we have it properly modeled in LPDB.
-        self.temproot = os.path.join(
-            self.distroroot, '%s-temp' % self.distroName)
 
     def setupArchiveDirs(self):
         """Create missing required directories in archive.

=== modified file 'lib/lp/archivepublisher/tests/test_ftparchive.py'
--- lib/lp/archivepublisher/tests/test_ftparchive.py	2010-10-17 09:03:43 +0000
+++ lib/lp/archivepublisher/tests/test_ftparchive.py	2010-12-18 00:21:49 +0000
@@ -20,7 +20,7 @@
     QuietFakeLogger,
     )
 from canonical.testing.layers import LaunchpadZopelessLayer
-from lp.archivepublisher.config import Config
+from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.diskpool import DiskPool
 from lp.archivepublisher.ftparchive import (
     f_touch,
@@ -71,7 +71,7 @@
 
         self._distribution = getUtility(IDistributionSet)['ubuntutest']
         self._archive = self._distribution.main_archive
-        self._config = Config(self._distribution)
+        self._config = getPubConfig(self._archive)
         self._config.setupArchiveDirs()
         self._sampledir = os.path.join(
             config.root, "lib", "lp", "archivepublisher", "tests",

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2010-12-02 15:35:38 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2010-12-18 00:21:49 +0000
@@ -1142,7 +1142,7 @@
         self.ubuntutest.getSeries('breezy-autotest').status = (
             SeriesStatus.FROZEN)
 
-        self.config = Config(self.ubuntutest)
+        self.config = getPubConfig(self.ubuntutest.main_archive)
         publisher = Publisher(
             self.logger, self.config, self.disk_pool,
             self.ubuntutest.main_archive)
@@ -1163,7 +1163,7 @@
 
         ds = self.ubuntutest.getSeries('breezy-autotest')
         ds.getDistroArchSeries('i386').enabled = False
-        self.config = Config(self.ubuntutest)
+        self.config = getPubConfig(self.ubuntutest.main_archive)
 
         publisher = Publisher(
             self.logger, self.config, self.disk_pool,

=== added file 'lib/lp/code/interfaces/branchmergequeue.py'
--- lib/lp/code/interfaces/branchmergequeue.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/interfaces/branchmergequeue.py	2010-11-03 08:28:44 +0000
@@ -0,0 +1,129 @@
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Branch merge queue interfaces."""
+
+__metaclass__ = type
+
+__all__ = [
+    'IBranchMergeQueue',
+    'IBranchMergeQueueSource',
+    'user_has_special_merge_queue_access',
+    ]
+
+from lazr.restful.declarations import (
+    export_as_webservice_entry,
+    export_write_operation,
+    exported,
+    mutator_for,
+    operation_parameters,
+    )
+from lazr.restful.fields import (
+    CollectionField,
+    Reference,
+    )
+from zope.component import getUtility
+from zope.interface import Interface
+from zope.schema import (
+    Datetime,
+    Int,
+    Text,
+    TextLine,
+    )
+
+from canonical.launchpad import _
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from lp.services.fields import (
+    PersonChoice,
+    PublicPersonChoice,
+    )
+
+
+class IBranchMergeQueue(Interface):
+    """An interface for managing branch merges."""
+
+    export_as_webservice_entry()
+
+    id = Int(title=_('ID'), readonly=True, required=True)
+
+    registrant = exported(
+        PublicPersonChoice(
+            title=_("The user that registered the branch."),
+            required=True, readonly=True,
+            vocabulary='ValidPersonOrTeam'))
+
+    owner = exported(
+        PersonChoice(
+            title=_('Owner'),
+            required=True, readonly=True,
+            vocabulary='UserTeamsParticipationPlusSelf',
+            description=_("The owner of the merge queue.")))
+
+    name = exported(
+        TextLine(
+            title=_('Name'), required=True,
+            description=_(
+                "Keep very short, unique, and descriptive, because it will "
+                "be used in URLs.  "
+                "Examples: main, devel, release-1.0, gnome-vfs.")))
+
+    description = exported(
+        Text(
+            title=_('Description'), required=False,
+            description=_(
+                'A short description of the purpose of this merge queue.')))
+
+    configuration = exported(
+        TextLine(
+            title=_('Configuration'), required=False, readonly=True,
+            description=_(
+                "A JSON string of configuration values.")))
+
+    date_created = exported(
+        Datetime(
+            title=_('Date Created'),
+            required=True,
+            readonly=True))
+
+    branches = exported(
+        CollectionField(
+            title=_('Dependent Branches'),
+            description=_(
+                'A collection of branches that this queue manages.'),
+            readonly=True,
+            value_type=Reference(Interface)))
+
+    @mutator_for(configuration)
+    @operation_parameters(
+        config=TextLine(title=_("A JSON string of configuration values.")))
+    @export_write_operation()
+    def setMergeQueueConfig(config):
+        """Set the JSON string configuration of the merge queue.
+
+        :param config: A JSON string of configuration values.
+        """
+
+
+class IBranchMergeQueueSource(Interface):
+
+    def new(name, owner, registrant, description, configuration, branches):
+        """Create a new IBranchMergeQueue object.
+
+        :param name: The name of the branch merge queue.
+        :param description: A description of queue.
+        :param configuration: A JSON string of configuration values.
+        :param owner: The owner of the queue.
+        :param registrant: The registrant of the queue.
+        :param branches: A list of branches to add to the queue.
+        """
+
+
+def user_has_special_merge_queue_access(user):
+    """Admins and bazaar experts have special access.
+
+    :param user: A 'Person' or None.
+    """
+    if user is None:
+        return False
+    celebs = getUtility(ILaunchpadCelebrities)
+    return user.inTeam(celebs.admin) or user.inTeam(celebs.bazaar_experts)

=== added file 'lib/lp/code/model/tests/test_branchmergequeue.py'
--- lib/lp/code/model/tests/test_branchmergequeue.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/model/tests/test_branchmergequeue.py	2010-10-25 14:51:56 +0000
@@ -0,0 +1,155 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for methods of BranchMergeQueue."""
+
+from __future__ import with_statement
+
+import simplejson
+
+from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.launchpad.webapp.testing import verifyObject
+from canonical.testing.layers import (
+    AppServerLayer,
+    DatabaseFunctionalLayer,
+    )
+from lp.code.errors import InvalidMergeQueueConfig
+from lp.code.interfaces.branchmergequeue import IBranchMergeQueue
+from lp.code.model.branchmergequeue import BranchMergeQueue
+from lp.testing import (
+    ANONYMOUS,
+    person_logged_in,
+    launchpadlib_for,
+    TestCaseWithFactory,
+    ws_object,
+    )
+
+
+class TestBranchMergeQueueInterface(TestCaseWithFactory):
+    """Test IBranchMergeQueue interface."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_implements_interface(self):
+        queue = self.factory.makeBranchMergeQueue()
+        IStore(BranchMergeQueue).add(queue)
+        verifyObject(IBranchMergeQueue, queue)
+
+
+class TestBranchMergeQueueSource(TestCaseWithFactory):
+    """Test the methods of IBranchMergeQueueSource."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_new(self):
+        owner = self.factory.makePerson()
+        name = u'SooperQueue'
+        description = u'This is Sooper Queue'
+        config = unicode(simplejson.dumps({'test': 'make check'}))
+
+        queue = BranchMergeQueue.new(
+            name, owner, owner, description, config)
+
+        self.assertEqual(queue.name, name)
+        self.assertEqual(queue.owner, owner)
+        self.assertEqual(queue.registrant, owner)
+        self.assertEqual(queue.description, description)
+        self.assertEqual(queue.configuration, config)
+
+
+class TestBranchMergeQueue(TestCaseWithFactory):
+    """Test the functions of the BranchMergeQueue."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_branches(self):
+        """Test that a merge queue can get all its managed branches."""
+        store = IStore(BranchMergeQueue)
+
+        queue = self.factory.makeBranchMergeQueue()
+        store.add(queue)
+
+        branch = self.factory.makeBranch()
+        store.add(branch)
+        with person_logged_in(branch.owner):
+            branch.addToQueue(queue)
+
+        self.assertEqual(
+            list(queue.branches),
+            [branch])
+
+    def test_setMergeQueueConfig(self):
+        """Test that the configuration is set properly."""
+        queue = self.factory.makeBranchMergeQueue()
+        config = unicode(simplejson.dumps({
+            'test': 'make test'}))
+
+        with person_logged_in(queue.owner):
+            queue.setMergeQueueConfig(config)
+
+        self.assertEqual(queue.configuration, config)
+
+    def test_setMergeQueueConfig_invalid_json(self):
+        """Test that invalid json can't be set as the config."""
+        queue = self.factory.makeBranchMergeQueue()
+
+        with person_logged_in(queue.owner):
+            self.assertRaises(
+                InvalidMergeQueueConfig,
+                queue.setMergeQueueConfig,
+                'abc')
+
+
+class TestWebservice(TestCaseWithFactory):
+
+    layer = AppServerLayer
+
+    def test_properties(self):
+        """Test that the correct properties are exposed."""
+        with person_logged_in(ANONYMOUS):
+            name = u'teh-queue'
+            description = u'Oh hai! I are a queues'
+            configuration = unicode(simplejson.dumps({'test': 'make check'}))
+
+            queuer = self.factory.makePerson()
+            db_queue = self.factory.makeBranchMergeQueue(
+                registrant=queuer, owner=queuer, name=name,
+                description=description,
+                configuration=configuration)
+            branch1 = self.factory.makeBranch()
+            with person_logged_in(branch1.owner):
+                branch1.addToQueue(db_queue)
+            branch2 = self.factory.makeBranch()
+            with person_logged_in(branch2.owner):
+                branch2.addToQueue(db_queue)
+            launchpad = launchpadlib_for('test', db_queue.owner,
+                service_root="http://api.launchpad.dev:8085")
+
+        queuer = ws_object(launchpad, queuer)
+        queue = ws_object(launchpad, db_queue)
+        branch1 = ws_object(launchpad, branch1)
+        branch2 = ws_object(launchpad, branch2)
+
+        self.assertEqual(queue.registrant, queuer)
+        self.assertEqual(queue.owner, queuer)
+        self.assertEqual(queue.name, name)
+        self.assertEqual(queue.description, description)
+        self.assertEqual(queue.configuration, configuration)
+        self.assertEqual(queue.date_created, db_queue.date_created)
+        self.assertEqual(len(queue.branches), 2)
+
+    def test_set_configuration(self):
+        """Test the mutator for setting configuration."""
+        with person_logged_in(ANONYMOUS):
+            db_queue = self.factory.makeBranchMergeQueue()
+            launchpad = launchpadlib_for('test', db_queue.owner,
+                service_root="http://api.launchpad.dev:8085")
+
+        configuration = simplejson.dumps({'test': 'make check'})
+
+        queue = ws_object(launchpad, db_queue)
+        queue.configuration = configuration
+        queue.lp_save()
+
+        queue2 = ws_object(launchpad, db_queue)
+        self.assertEqual(queue2.configuration, configuration)

=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
--- lib/lp/soyuz/tests/test_publishing.py	2010-12-15 06:03:14 +0000
+++ lib/lp/soyuz/tests/test_publishing.py	2010-12-18 00:21:49 +0000
@@ -26,7 +26,7 @@
     )
 from canonical.testing.layers import reconnect_stores
 from lp.app.errors import NotFoundError
-from lp.archivepublisher.config import Config
+from lp.archivepublisher.config import getPubConfig
 from lp.archivepublisher.diskpool import DiskPool
 from lp.buildmaster.enums import BuildStatus
 from lp.registry.interfaces.distribution import IDistributionSet
@@ -571,7 +571,7 @@
         super(TestNativePublishingBase, self).setUp()
         self.layer.switchDbUser(config.archivepublisher.dbuser)
         self.prepareBreezyAutotest()
-        self.config = Config(self.ubuntutest)
+        self.config = getPubConfig(self.ubuntutest.main_archive)
         self.config.setupArchiveDirs()
         self.pool_dir = self.config.poolroot
         self.temp_dir = self.config.temproot