Merge lp:~mwhudson/launchpad/no-hosted-area-remove-mirrorComplete into lp:launchpad

[Michael Hudson-Doyle]

Hi Tim yet again!

This branch removes the now obsolete mirrorComplete branch method and also removes the long-obsolete 'startMirroring' method from the XML-RPC endpoint (it's still a branch method: acquireBranchToPull calls it).

Cheers,
mwh

[Tim Penhey (thumper)]

We must be getting close now...

review: Approve

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-04-27 02:05:59 +0000
+++ lib/lp/code/configure.zcml	2010-04-27 02:06:23 +0000
@@ -521,7 +521,6 @@
                     getPullURL
                     requestMirror
                     startMirroring
-                    mirrorComplete
                     mirrorFailed
                     branch_format
                     repository_format
 
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/interfaces/branch.py	2010-04-27 02:06:23 +0000
@@ -1105,13 +1105,6 @@
             branch.
         """
 
-    def mirrorComplete(last_revision_id):
-        """Signal that a mirror attempt has completed successfully.
-
-        :param last_revision_id: The revision ID of the tip of the mirrored
-            branch.
-        """
-
     def mirrorFailed(reason):
         """Signal that a mirror attempt failed.
 
 
=== modified file 'lib/lp/code/interfaces/codehosting.py'
--- lib/lp/code/interfaces/codehosting.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/interfaces/codehosting.py	2010-04-27 02:06:23 +0000
@@ -82,30 +82,6 @@
             or (), the empty tuple, if there is no branch to pull.
         """
 
-    def startMirroring(branchID):
-        """Notify Launchpad that the given branch has started mirroring.
-
-        The last_mirror_attempt field of the given branch record will be
-        updated appropriately.
-
-        :param branchID: The database ID of the given branch.
-        :returns: True if the branch status was successfully updated.
-            `NoBranchWithID` fault if there's no branch with the given id.
-        """
-
-    def mirrorComplete(branchID, lastRevisionID):
-        """Notify Launchpad that the branch has been successfully mirrored.
-
-        In the Launchpad database, the last_mirrored field will be updated to
-        match the last_mirror_attempt value, the mirror_failures counter will
-        be reset to zero and the next_mirror_time will be set to NULL.
-
-        :param branchID: The database ID of the given branch.
-        :param lastRevisionID: The last revision ID mirrored.
-        :returns: True if the branch status was successfully updated.
-            `NoBranchWithID` fault if there's no branch with the given id.
-        """
-
     def mirrorFailed(branchID, reason):
         """Notify Launchpad that the branch could not be mirrored.
 
 
=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py	2010-04-27 02:06:23 +0000
@@ -872,7 +872,8 @@
         target_tree.commit('rev1')
         # Make sure that the created branch has been mirrored.
         db_target_branch.startMirroring()
-        db_target_branch.mirrorComplete('rev1')
+        removeSecurityProxy(db_target_branch).branchChanged(
+            '', 'rev1', None, None, None)
         source_tree = target_tree.bzrdir.sprout('source').open_workingtree()
         source_tree.commit('rev2')
         message = self.factory.makeBundleMergeDirectiveEmail(
@@ -1000,7 +1001,8 @@
         target_tree.commit('rev1')
         # Make sure that the created branch has been mirrored.
         db_target_branch.startMirroring()
-        db_target_branch.mirrorComplete('rev1')
+        removeSecurityProxy(db_target_branch).branchChanged(
+            '', 'rev1', None, None, None)
 
         db_source_branch, source_tree = self.create_branch_and_tree(
             'lpsource', db_target_branch.product, hosted=True,
 
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/model/branch.py	2010-04-27 02:06:23 +0000
@@ -832,6 +832,8 @@
         new_data_pushed = (
              self.branch_type in (BranchType.HOSTED, BranchType.IMPORTED)
              and self.next_mirror_time is not None)
+        # XXX 2010-04-22, MichaelHudson: This should really look for a branch
+        # scan job.
         pulled_but_not_scanned = self.last_mirrored_id != self.last_scanned_id
         pull_in_progress = (
             self.last_mirror_attempt is not None
@@ -931,25 +933,6 @@
         self.branch_format = branch_format
         self.repository_format = repository_format
 
-    def mirrorComplete(self, last_revision_id):
-        """See `IBranch`."""
-        if self.branch_type == BranchType.REMOTE:
-            raise BranchTypeError(self.unique_name)
-        assert self.last_mirror_attempt != None, (
-            "startMirroring must be called before mirrorComplete.")
-        self.last_mirrored = self.last_mirror_attempt
-        self.mirror_failures = 0
-        self.mirror_status_message = None
-        if (self.next_mirror_time is None
-            and self.branch_type == BranchType.MIRRORED):
-            # No mirror was requested since we started mirroring.
-            increment = getUtility(IBranchPuller).MIRROR_TIME_INCREMENT
-            self.next_mirror_time = (
-                datetime.now(pytz.timezone('UTC')) + increment)
-        self.last_mirrored_id = last_revision_id
-        from lp.code.model.branchjob import BranchScanJob
-        BranchScanJob.create(self)
-
     def mirrorFailed(self, reason):
         """See `IBranch`."""
         if self.branch_type == BranchType.REMOTE:
 
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/model/tests/test_branch.py	2010-04-27 02:06:23 +0000
@@ -1794,28 +1794,18 @@
         branch = self.factory.makeAnyBranch()
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
-        branch.mirrorComplete(rev_id)
+        removeSecurityProxy(branch).branchChanged(
+            '', rev_id, None, None, None)
         self.assertEqual(True, branch.pending_writes)
 
-    def test_mirrorComplete_creates_scan_job(self):
-        # After a branch has been pulled, it should have created a
-        # BranchScanJob to complete the process.
-        branch = self.factory.makeAnyBranch()
-        branch.startMirroring()
-        rev_id = self.factory.getUniqueString('rev-id')
-        branch.mirrorComplete(rev_id)
-
-        store = Store.of(branch)
-        scan_jobs = store.find(BranchJob, job_type=BranchJobType.SCAN_BRANCH)
-        self.assertEqual(scan_jobs.count(), 1)
-
     def test_pulled_and_scanned(self):
         # If a branch has been pulled and scanned, then there are no pending
         # writes.
         branch = self.factory.makeAnyBranch()
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
-        branch.mirrorComplete(rev_id)
+        removeSecurityProxy(branch).branchChanged(
+            '', rev_id, None, None, None)
         # Cheat! The actual API for marking a branch as scanned is
         # updateScannedDetails. That requires a revision in the database
         # though.
@@ -1835,7 +1825,8 @@
         branch = self.factory.makeAnyBranch()
         branch.startMirroring()
         rev_id = self.factory.getUniqueString('rev-id')
-        branch.mirrorComplete(rev_id)
+        removeSecurityProxy(branch).branchChanged(
+            '', rev_id, None, None, None)
         # Cheat! The actual API for marking a branch as scanned is
         # updateScannedDetails. That requires a revision in the database
         # though.
 
=== modified file 'lib/lp/code/model/tests/test_branchpuller.py'
--- lib/lp/code/model/tests/test_branchpuller.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/model/tests/test_branchpuller.py	2010-04-27 02:06:23 +0000
@@ -84,7 +84,8 @@
         branch.requestMirror()
         transaction.commit()
         branch.startMirroring()
-        branch.mirrorComplete('rev1')
+        removeSecurityProxy(branch).branchChanged(
+            '', 'rev1', None, None, None)
         self.assertEqual(None, branch.next_mirror_time)
 
     def test_mirrorFailureResetsMirrorRequest(self):
@@ -152,7 +153,8 @@
         branch.requestMirror()
         transaction.commit()
         branch.startMirroring()
-        branch.mirrorComplete('rev1')
+        removeSecurityProxy(branch).branchChanged(
+            '', 'rev1', None, None, None)
         self.assertInFuture(branch.next_mirror_time, self.increment)
         self.assertEqual(0, branch.mirror_failures)
 
 
=== modified file 'lib/lp/code/model/tests/test_branchtarget.py'
--- lib/lp/code/model/tests/test_branchtarget.py	2010-04-13 15:08:13 +0000
+++ lib/lp/code/model/tests/test_branchtarget.py	2010-04-27 02:06:23 +0000
@@ -108,7 +108,8 @@
         default_branch = self.factory.makePackageBranch(
             sourcepackage=development_package)
         default_branch.startMirroring()
-        default_branch.mirrorComplete(self.factory.getUniqueString())
+        removeSecurityProxy(default_branch).branchChanged(
+            '', self.factory.getUniqueString(), None, None, None)
         ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
         run_with_login(
             ubuntu_branches.teamowner,
@@ -347,7 +348,8 @@
         branch = self.factory.makeProductBranch(product=self.original)
         self._setDevelopmentFocus(self.original, branch)
         branch.startMirroring()
-        branch.mirrorComplete('rev1')
+        removeSecurityProxy(branch).branchChanged(
+            '', 'rev1', None, None, None)
         target = IBranchTarget(self.original)
         self.assertEqual(branch, target.default_stacked_on_branch)
 
@@ -450,7 +452,8 @@
         # life.
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         branch.startMirroring()
-        branch.mirrorComplete(self.factory.getUniqueString())
+        removeSecurityProxy(branch).branchChanged(
+            '', self.factory.getUniqueString(), None, None, None)
         removeSecurityProxy(branch).branch_type = BranchType.REMOTE
         self.assertIs(None, check_default_stacked_on(branch))
 
@@ -466,7 +469,8 @@
         branch = self.factory.makeAnyBranch(private=True)
         naked_branch = removeSecurityProxy(branch)
         naked_branch.startMirroring()
-        naked_branch.mirrorComplete(self.factory.getUniqueString())
+        naked_branch.branchChanged(
+            '', self.factory.getUniqueString(), None, None, None)
         self.assertIs(None, check_default_stacked_on(branch))
 
     def test_been_mirrored(self):
@@ -475,7 +479,8 @@
         # futile.
         branch = self.factory.makeAnyBranch()
         branch.startMirroring()
-        branch.mirrorComplete('rev1')
+        removeSecurityProxy(branch).branchChanged(
+            '', self.factory.getUniqueString(), None, None, None)
         self.assertEqual(branch, check_default_stacked_on(branch))
 
 
 
=== modified file 'lib/lp/code/stories/branches/xx-branch-mirror-failures.txt'
--- lib/lp/code/stories/branches/xx-branch-mirror-failures.txt	2009-06-12 16:36:02 +0000
+++ lib/lp/code/stories/branches/xx-branch-mirror-failures.txt	2010-04-27 02:06:23 +0000
@@ -163,7 +163,7 @@
     >>> login(ANONYMOUS)
     >>> mirror_branch = getUtility(IBranchLookup).getByUniqueName(mirror_name)
     >>> mirror_branch.startMirroring()
-    >>> mirror_branch.mirrorComplete('some-revision-id')
+    >>> mirror_branch.branchChanged('', 'some-revision-id', None, None, None)
     >>> logout()
 
     >>> browser.open(branch_location)
 
=== modified file 'lib/lp/code/xmlrpc/codehosting.py'
--- lib/lp/code/xmlrpc/codehosting.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/xmlrpc/codehosting.py	2010-04-27 02:06:23 +0000
@@ -123,22 +123,13 @@
         else:
             return ()
 
-    def mirrorComplete(self, branch_id, last_revision_id):
-        """See `ICodehostingAPI`."""
-        branch = getUtility(IBranchLookup).get(branch_id)
-        if branch is None:
-            return faults.NoBranchWithID(branch_id)
-        # See comment in startMirroring.
-        branch = removeSecurityProxy(branch)
-        branch.mirrorComplete(last_revision_id)
-        return True
-
     def mirrorFailed(self, branch_id, reason):
         """See `ICodehostingAPI`."""
         branch = getUtility(IBranchLookup).get(branch_id)
         if branch is None:
             return faults.NoBranchWithID(branch_id)
-        # See comment in startMirroring.
+        # The puller runs as no user and may pull private branches. We need to
+        # bypass Zope's security proxy to set the mirroring information.
         removeSecurityProxy(branch).mirrorFailed(reason)
         return True
 
@@ -151,16 +142,6 @@
             date_completed=date_completed, hostname=hostname)
         return True
 
-    def startMirroring(self, branch_id):
-        """See `ICodehostingAPI`."""
-        branch = getUtility(IBranchLookup).get(branch_id)
-        if branch is None:
-            return faults.NoBranchWithID(branch_id)
-        # The puller runs as no user and may pull private branches. We need to
-        # bypass Zope's security proxy to set the mirroring information.
-        removeSecurityProxy(branch).startMirroring()
-        return True
-
     def createBranch(self, login_id, branch_path):
         """See `ICodehostingAPI`."""
         def create_branch(requester):
 
=== modified file 'lib/lp/code/xmlrpc/tests/test_codehosting.py'
--- lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-27 02:05:59 +0000
+++ lib/lp/code/xmlrpc/tests/test_codehosting.py	2010-04-27 02:06:23 +0000
@@ -179,34 +179,17 @@
         self.assertIs(self.branch_lookup.get(branch_id), None)
         return branch_id
 
-    def test_startMirroring(self):
-        # startMirroring updates last_mirror_attempt to 'now', leaves
-        # last_mirrored alone and returns True when passed the id of an
-        # existing branch.
-        branch = self.factory.makeAnyBranch()
-        self.assertUnmirrored(branch)
-
-        success = self.codehosting_api.startMirroring(branch.id)
-        self.assertEqual(success, True)
-
-        self.assertSqlAttributeEqualsDate(
-            branch, 'last_mirror_attempt', UTC_NOW)
-        self.assertIs(None, branch.last_mirrored)
-
-    def test_startMirroringInvalidBranch(self):
-        # startMirroring returns False when given a branch id which does not
-        # exist.
-        invalid_id = self.getUnusedBranchID()
-        fault = self.codehosting_api.startMirroring(invalid_id)
-        self.assertEqual(faults.NoBranchWithID(invalid_id), fault)
-
     def test_mirrorFailed(self):
-        branch = self.factory.makeAnyBranch()
+        branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
         self.assertUnmirrored(branch)
 
-        self.codehosting_api.startMirroring(branch.id)
+        branch.requestMirror()
+        self.assertEquals(
+            branch.id, self.codehosting_api.acquireBranchToPull([])[0])
+
         failure_message = self.factory.getUniqueString()
-        success = self.codehosting_api.mirrorFailed(branch.id, failure_message)
+        success = self.codehosting_api.mirrorFailed(
+            branch.id, failure_message)
         self.assertEqual(True, success)
         self.assertMirrorFailed(branch, failure_message)
 
@@ -216,60 +199,6 @@
         fault = self.codehosting_api.mirrorFailed(branch_id, failure_message)
         self.assertEqual(faults.NoBranchWithID(branch_id), fault)
 
-    def test_mirrorComplete(self):
-        # mirrorComplete marks the branch as having been successfully
-        # mirrored, with no failures and no status message.
-        branch = self.factory.makeAnyBranch()
-        self.assertUnmirrored(branch)
-
-        self.codehosting_api.startMirroring(branch.id)
-        revision_id = self.factory.getUniqueString()
-        success = self.codehosting_api.mirrorComplete(branch.id, revision_id)
-        self.assertEqual(True, success)
-        self.assertMirrorSucceeded(branch, revision_id)
-
-    def test_mirrorCompleteWithNoBranchID(self):
-        # mirrorComplete returns a Fault if there's no branch with the given
-        # ID.
-        branch_id = self.getUnusedBranchID()
-        fault = self.codehosting_api.mirrorComplete(
-            branch_id, self.factory.getUniqueString())
-        self.assertEqual(faults.NoBranchWithID(branch_id), fault)
-
-    def test_mirrorComplete_resets_failure_count(self):
-        # mirrorComplete marks the branch as successfully mirrored and removes
-        # all memory of failure.
-
-        # First, mark the branch as failed.
-        branch = self.factory.makeAnyBranch()
-        self.codehosting_api.startMirroring(branch.id)
-        failure_message = self.factory.getUniqueString()
-        self.codehosting_api.mirrorFailed(branch.id, failure_message)
-        self.assertMirrorFailed(branch, failure_message)
-
-        # Start and successfully finish a mirror.
-        self.codehosting_api.startMirroring(branch.id)
-        revision_id = self.factory.getUniqueString()
-        self.codehosting_api.mirrorComplete(branch.id, revision_id)
-
-        # Confirm that it succeeded.
-        self.assertMirrorSucceeded(branch, revision_id)
-
-    def test_mirrorComplete_resets_mirror_request(self):
-        # After successfully mirroring a hosted branch, next_mirror_time
-        # should be set to NULL.
-        branch = self.factory.makeAnyBranch(branch_type=BranchType.HOSTED)
-
-        # Request that branch be mirrored. This sets next_mirror_time.
-        branch.requestMirror()
-
-        # Simulate successfully mirroring the branch.
-        self.codehosting_api.startMirroring(branch.id)
-        self.codehosting_api.mirrorComplete(
-            branch.id, self.factory.getUniqueString())
-
-        self.assertIs(None, branch.next_mirror_time)
-
     def test_recordSuccess(self):
         # recordSuccess must insert the given data into ScriptActivity.
         started = datetime.datetime(2007, 07, 05, 19, 32, 1, tzinfo=UTC)
@@ -939,7 +868,10 @@
 
     def startMirroring(self, branch):
         """See `AcquireBranchToPullTests`."""
-        self.codehosting_api.startMirroring(branch.id)
+        # This is a bit random, but it works.  acquireBranchToPull marks the
+        # branch it returns as started mirroring, but we should check that the
+        # one we want is returned...
+        self.assertBranchIsAquired(branch, branch.branch_type)
 
     def test_branch_type_returned_mirrored(self):
         branch = self.factory.makeAnyBranch(branch_type=BranchType.MIRRORED)
 
=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2010-04-27 02:05:59 +0000
+++ lib/lp/codehosting/inmemory.py	2010-04-27 02:06:23 +0000
@@ -480,7 +480,9 @@
             key=operator.attrgetter('next_mirror_time'))
         if branches:
             branch = branches[-1]
-            self.startMirroring(branch.id)
+            # Mark it as started mirroring.
+            branch.last_mirror_attempt = UTC_NOW
+            branch.next_mirror_time = None
             default_branch = branch.target.default_stacked_on_branch
             if default_branch is None:
                 default_branch_name = ''
@@ -494,26 +496,6 @@
         else:
             return ()
 
-    def startMirroring(self, branch_id):
-        branch = self._branch_set.get(branch_id)
-        if branch is None:
-            return faults.NoBranchWithID(branch_id)
-        branch.last_mirror_attempt = UTC_NOW
-        branch.next_mirror_time = None
-        return True
-
-    def mirrorComplete(self, branch_id, last_revision_id):
-        branch = self._branch_set.get(branch_id)
-        if branch is None:
-            return faults.NoBranchWithID(branch_id)
-        branch.last_mirrored_id = last_revision_id
-        branch.last_mirrored = UTC_NOW
-        branch.mirror_failures = 0
-        for stacked_branch in self._branch_set:
-            if stacked_branch.stacked_on is branch:
-                stacked_branch.requestMirror()
-        return True
-
     def mirrorFailed(self, branch_id, reason):
         branch = self._branch_set.get(branch_id)
         if branch is None:
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-04-27 02:05:59 +0000
+++ lib/lp/testing/factory.py	2010-04-27 02:06:23 +0000
@@ -863,13 +863,10 @@
         """
         if branch is None:
             branch = self.makeBranch(product=product)
-        # 'branch' might be private, so we remove the security proxy to get at
-        # the methods.
-        naked_branch = removeSecurityProxy(branch)
-        naked_branch.startMirroring()
-        naked_branch.mirrorComplete('rev1')
-        # Likewise, we might not have permission to set the branch of the
-        # development focus series.
+        # We just remove the security proxies to be able to change the objects
+        # here.
+        removeSecurityProxy(branch).branchChanged(
+            '', 'rev1', None, None, None)
         naked_series = removeSecurityProxy(product.development_focus)
         naked_series.branch = branch
         return branch
@@ -881,11 +878,10 @@
         :param branch: The branch that should be the default stacked-on
             branch.
         """
-        # 'branch' might be private, so we remove the security proxy to get at
-        # the methods.
-        naked_branch = removeSecurityProxy(branch)
-        naked_branch.startMirroring()
-        naked_branch.mirrorComplete('rev1')
+        # We just remove the security proxies to be able to change the branch
+        # here.
+        removeSecurityProxy(branch).branchChanged(
+            '', 'rev1', None, None, None)
         ubuntu_branches = getUtility(ILaunchpadCelebrities).ubuntu_branches
         run_with_login(
             ubuntu_branches.teamowner,