launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:stormify-builder into launchpad:master.

Commit message:
Convert Builder to Storm

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/395397
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:stormify-builder into launchpad:master.

diff --git a/lib/lp/buildmaster/doc/builder.txt b/lib/lp/buildmaster/doc/builder.txt
index 4290a2f..e8249ad 100644
--- a/lib/lp/buildmaster/doc/builder.txt
+++ b/lib/lp/buildmaster/doc/builder.txt
@@ -14,7 +14,8 @@ packages.
 There are several builders in the sample data. Let's examine the first.
 
     >>> from lp.buildmaster.model.builder import Builder
-    >>> builder = Builder.get(1)
+    >>> from lp.services.database.interfaces import IStore
+    >>> builder = IStore(Builder).get(Builder, 1)
 
 As expected, it implements IBuilder.
 
@@ -65,7 +66,7 @@ And also by ID.
     >>> print(builderset.get(100).name)
     Traceback (most recent call last):
     ...
-    SQLObjectNotFound: Object not found
+    NotFoundError: 100
 
 The 'new' method will create a new builder in the database.
 
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index 7bbe4aa..64fdb21 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -409,7 +409,7 @@ def recover_failure(logger, vitals, builder, retry, exception):
         # We've already tried resetting it enough times, so we have
         # little choice but to give up.
         logger.info("Failing builder %s.", builder.name)
-        builder.failBuilder(str(exception))
+        builder.failBuilder(six.text_type(exception))
     elif builder_action == True:
         # Dirty the builder to attempt recovery. In the virtual case,
         # the dirty idleness will cause a reset, giving us a good chance
diff --git a/lib/lp/buildmaster/model/builder.py b/lib/lp/buildmaster/model/builder.py
index 93326ee..5e66baf 100644
--- a/lib/lp/buildmaster/model/builder.py
+++ b/lib/lp/buildmaster/model/builder.py
@@ -9,19 +9,18 @@ __all__ = [
     'BuilderSet',
     ]
 
-from sqlobject import (
-    BoolCol,
-    ForeignKey,
-    IntCol,
-    SQLObjectNotFound,
-    StringCol,
-    )
+import pytz
 from storm.expr import (
     Coalesce,
     Count,
     Sum,
     )
-from storm.properties import Int
+from storm.properties import (
+    Bool,
+    DateTime,
+    Int,
+    Unicode,
+    )
 from storm.references import Reference
 from storm.store import Store
 from zope.component import getUtility
@@ -50,14 +49,12 @@ from lp.services.database.bulk import (
     load_related,
     )
 from lp.services.database.constants import UTC_NOW
-from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.decoratedresultset import DecoratedResultSet
-from lp.services.database.enumcol import EnumCol
+from lp.services.database.enumcol import DBEnum
 from lp.services.database.interfaces import (
     ISlaveStore,
     IStore,
     )
-from lp.services.database.sqlbase import SQLBase
 from lp.services.database.stormbase import StormBase
 from lp.services.propertycache import (
     cachedproperty,
@@ -71,41 +68,64 @@ from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
 
 
 @implementer(IBuilder, IHasBuildRecords)
-class Builder(SQLBase):
-    _table = 'Builder'
-
-    _defaultOrder = ['id']
-
-    url = StringCol(dbName='url', notNull=True)
-    name = StringCol(dbName='name', notNull=True)
-    title = StringCol(dbName='title', notNull=True)
-    owner = ForeignKey(
-        dbName='owner', foreignKey='Person',
-        storm_validator=validate_public_person, notNull=True)
-    _builderok = BoolCol(dbName='builderok', notNull=True)
-    failnotes = StringCol(dbName='failnotes')
-    virtualized = BoolCol(dbName='virtualized', default=True, notNull=True)
-    manual = BoolCol(dbName='manual', default=False)
-    vm_host = StringCol(dbName='vm_host')
-    active = BoolCol(dbName='active', notNull=True, default=True)
-    failure_count = IntCol(dbName='failure_count', default=0, notNull=True)
-    version = StringCol(dbName='version')
-    clean_status = EnumCol(
+class Builder(StormBase):
+    __storm_table__ = 'Builder'
+    __storm_order__ = ['id']
+
+    id = Int(primary=True)
+    url = Unicode(name='url', allow_none=False)
+    name = Unicode(name='name', allow_none=False)
+    title = Unicode(name='title', allow_none=False)
+    owner_id = Int(
+        name='owner', validator=validate_public_person, allow_none=False)
+    owner = Reference(owner_id, 'Person.id')
+    _builderok = Bool(name='builderok', allow_none=False)
+    failnotes = Unicode(name='failnotes')
+    virtualized = Bool(name='virtualized', default=True, allow_none=False)
+    manual = Bool(name='manual', default=False)
+    vm_host = Unicode(name='vm_host')
+    active = Bool(name='active', allow_none=False, default=True)
+    failure_count = Int(name='failure_count', default=0, allow_none=False)
+    version = Unicode(name='version')
+    clean_status = DBEnum(
         enum=BuilderCleanStatus, default=BuilderCleanStatus.DIRTY)
-    vm_reset_protocol = EnumCol(enum=BuilderResetProtocol)
-    date_clean_status_changed = UtcDateTimeCol()
+    vm_reset_protocol = DBEnum(enum=BuilderResetProtocol)
+    date_clean_status_changed = DateTime(tzinfo=pytz.UTC)
+
+    def __init__(self, processors, url, name, title, owner, active=True,
+                 virtualized=True, vm_host=None, vm_reset_protocol=None,
+                 builderok=True, manual=False):
+        super(Builder, self).__init__()
+        # The processors cache starts out empty so that the processors
+        # property setter doesn't issue an additional query.
+        get_property_cache(self)._processors_cache = []
+        self.url = url
+        self.name = name
+        self.title = title
+        self.owner = owner
+        self.active = active
+        self.virtualized = virtualized
+        self.vm_host = vm_host
+        self.vm_reset_protocol = vm_reset_protocol
+        self._builderok = builderok
+        self.manual = manual
+        # We have to add the new object to the store here (it might more
+        # normally be done in BuilderSet.new), because the processors
+        # property setter needs to link other objects to it.
+        IStore(Builder).add(self)
+        self.processors = processors
 
-    def _getBuilderok(self):
+    @property
+    def builderok(self):
         return self._builderok
 
-    def _setBuilderok(self, value):
+    @builderok.setter
+    def builderok(self, value):
         self._builderok = value
         if value is True:
             self.resetFailureCount()
             self.setCleanStatus(BuilderCleanStatus.DIRTY)
 
-    builderok = property(_getBuilderok, _setBuilderok)
-
     def gotFailure(self):
         """See `IBuilder`."""
         self.failure_count += 1
@@ -124,10 +144,12 @@ class Builder(SQLBase):
             BuilderProcessor.processor_id == Processor.id,
             BuilderProcessor.builder == self).order_by(Processor.name))
 
-    def _processors(self):
+    @property
+    def processors(self):
         return self._processors_cache
 
-    def _set_processors(self, processors):
+    @processors.setter
+    def processors(self, processors):
         existing = set(self.processors)
         wanted = set(processors)
         # Enable the wanted but missing.
@@ -144,8 +166,6 @@ class Builder(SQLBase):
                 processor.id for processor in existing - wanted)).remove()
         del get_property_cache(self)._processors_cache
 
-    processors = property(_processors, _set_processors)
-
     @property
     def processor(self):
         """See `IBuilder`."""
@@ -218,14 +238,11 @@ class BuilderSet(object):
         self.title = "The Launchpad build farm"
 
     def __iter__(self):
-        return iter(Builder.select())
+        return iter(IStore(Builder).find(Builder))
 
     def getByName(self, name):
         """See IBuilderSet."""
-        try:
-            return Builder.selectOneBy(name=name)
-        except SQLObjectNotFound:
-            raise NotFoundError(name)
+        return IStore(Builder).find(Builder, name=name).one()
 
     def __getitem__(self, name):
         return self.getByName(name)
@@ -234,18 +251,22 @@ class BuilderSet(object):
             virtualized=False, vm_host=None, vm_reset_protocol=None,
             manual=True):
         """See IBuilderSet."""
-        return Builder(processors=processors, url=url, name=name, title=title,
-                       owner=owner, active=active, virtualized=virtualized,
-                       vm_host=vm_host, vm_reset_protocol=vm_reset_protocol,
-                       _builderok=True, manual=manual)
+        return Builder(
+            processors=processors, url=url, name=name, title=title,
+            owner=owner, active=active, virtualized=virtualized,
+            vm_host=vm_host, vm_reset_protocol=vm_reset_protocol,
+            builderok=True, manual=manual)
 
     def get(self, builder_id):
         """See IBuilderSet."""
-        return Builder.get(builder_id)
+        builder = IStore(Builder).get(Builder, builder_id)
+        if builder is None:
+            raise NotFoundError(builder_id)
+        return builder
 
     def count(self):
         """See IBuilderSet."""
-        return Builder.select().count()
+        return IStore(Builder).find(Builder).count()
 
     def preloadProcessors(self, builders):
         """See `IBuilderSet`."""
@@ -274,7 +295,7 @@ class BuilderSet(object):
 
         def preload(rows):
             self.preloadProcessors(rows)
-            load_related(Person, rows, ['ownerID'])
+            load_related(Person, rows, ['owner_id'])
             bqs = getUtility(IBuildQueueSet).preloadForBuilders(rows)
             BuildQueue.preloadSpecificBuild(bqs)
         return DecoratedResultSet(rs, pre_iter_hook=preload)
diff --git a/lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py b/lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py
index b846e87..ceec2a9 100644
--- a/lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py
+++ b/lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py
@@ -68,7 +68,7 @@ class TestSourcePackageRecipeBuildMailer(TestCaseWithFactory):
             status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=5))
         build.updateStatus(
             BuildStatus.FULLYBUILT,
-            builder=self.factory.makeBuilder(name='bob'))
+            builder=self.factory.makeBuilder(name=u'bob'))
         build.setLog(self.factory.makeLibraryFileAlias())
         ctrl = self.makeStatusEmail(build)
         self.assertEqual(
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index b38c7d6..685a142 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -3016,11 +3016,11 @@ class BareLaunchpadObjectFactory(ObjectFactory):
         if processors is None:
             processors = [getUtility(IProcessorSet).getByName('386')]
         if url is None:
-            url = 'http://%s:8221/' % self.getUniqueString()
+            url = 'http://%s:8221/' % self.getUniqueUnicode()
         if name is None:
-            name = self.getUniqueString('builder-name')
+            name = self.getUniqueUnicode('builder-name')
         if title is None:
-            title = self.getUniqueString('builder-title')
+            title = self.getUniqueUnicode('builder-title')
         if owner is None:
             owner = self.makePerson()
         if virtualized and vm_reset_protocol is None:
diff --git a/lib/lp/testing/sampledata.py b/lib/lp/testing/sampledata.py
index b314351..74d78d8 100644
--- a/lib/lp/testing/sampledata.py
+++ b/lib/lp/testing/sampledata.py
@@ -7,6 +7,8 @@ If ever you use a literal in a test that refers to sample data, even if it's
 just a small number, then you should define it as a constant here.
 """
 
+from __future__ import absolute_import, print_function, unicode_literals
+
 __metaclass__ = type
 __all__ = [
     'ADMIN_EMAIL',
diff --git a/lib/lp/translations/stories/buildfarm/xx-build-summary.txt b/lib/lp/translations/stories/buildfarm/xx-build-summary.txt
index 230b756..36c8e7b 100644
--- a/lib/lp/translations/stories/buildfarm/xx-build-summary.txt
+++ b/lib/lp/translations/stories/buildfarm/xx-build-summary.txt
@@ -51,7 +51,7 @@ Create a builder working on a TranslationTemplatesBuild for a branch.
     >>> unused = ubuntu.currentseries.nominatedarchindep.addOrUpdateChroot(
     ...     fake_chroot)
 
-    >>> builder = factory.makeBuilder(vm_host=factory.getUniqueString())
+    >>> builder = factory.makeBuilder(vm_host=factory.getUniqueUnicode())
     >>> _ = patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(FakeSlave()))
     >>> buildqueue.markAsBuilding(builder)