Merge lp:~robru/cupstream2distro/refactor-publish into lp:cupstream2distro

[Robert Bruce Park]

Move publisher.push_branches into Merge class.

[Robert Bruce Park (robru)]

Looks great in staging:

https://ci-train.staging.ubuntu.com/job/ubuntu-landing-014-1-build/3/consoleFull

Branch pushed:

https://code.launchpad.net/~untrusted-ci-dev-bot/ubuntu-system-settings/ubuntu-system-settings-ubuntu-wily-landing-014

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1171
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/840/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/840/rebuild

review: Approve (continuous-integration)

[lp:~robru/cupstream2distro/refactor-publish]

1168. By Robert Bruce Park on 2015-10-23

Move publisher.push_branches into Merge class.

Approved by PS Jenkins bot, Robert Bruce Park.

=== modified file 'citrain/build.py'
--- citrain/build.py	2015-10-20 19:25:31 +0000
+++ citrain/build.py	2015-10-22 23:24:39 +0000
@@ -86,7 +86,7 @@
         manager = buildmanager(silo_state)
         manager.do('validate')
         if env.WATCH_ONLY != 'true':
-            manager.do('clean', 'collect', 'build', 'upload')
+            manager.do('clean', 'collect', 'build', 'upload', 'expose')
         Manager(silo_state).do('watch', 'diff')
         silo_state.status = 'Packages built.'
     except CITrainError as err:

=== modified file 'citrain/publisher.py'
--- citrain/publisher.py	2015-10-22 18:56:16 +0000
+++ citrain/publisher.py	2015-10-22 23:24:39 +0000
@@ -48,21 +48,13 @@
 from citrain.recipes.manager import Manager
 from cupstream2distro.version import V
 from cupstream2distro.archive import Archive
-from cupstream2distro.branchhandling import Branch
 from cupstream2distro.packagelist import PackageList
 from cupstream2distro.errors import CITrainError, PublishError, NoStatusError
+from cupstream2distro.utils import env, log_value_of, run_script
+from cupstream2distro.settings import STABLE_OVERLAY_PPA
 from cupstream2distro.silomanager import SiloState
 from cupstream2distro.launchpadmanager import lp
 from cupstream2distro.project import DotProject
-from cupstream2distro.settings import (
-    STABLE_OVERLAY_PPA,
-)
-from cupstream2distro.utils import (
-    SILO_DIR,
-    env,
-    log_value_of,
-    run_script,
-)
 
 
 def get_distro_and_series(series):
@@ -73,18 +65,6 @@
         env.DISTRO, env.SERIES))
 
 
-# FIXME: Call this during the build job, so we have access to it sooner.
-def push_branches(mps):
-    """Push all local branches to Launchpad."""
-    for source in sorted(mps):
-        project_name = mps[source][0].target_branch.project.name
-        branch = SILO_DIR(source)
-        proposed = 'lp:~/{}/{}-{}-{}-proposed'.format(
-            project_name, source, env.DISTRO, env.SERIES)
-        logging.info('Pushing {} to {}'.format(source, proposed))
-        Branch(branch).push_to_branch(proposed, overwrite=True)
-
-
 def find_ppa_sources(ppa, sources, versions):
     """Populate sources set with all Published and Pending sources in a PPA.
 
@@ -173,7 +153,6 @@
         silo_state.status = 'Publishing.'
         silo_state.save_config()
         mgr.do('dest_version_check', 'unapproved', 'unbuilt')
-        push_branches(silo_state.mps)
         initiate_publication(dest, ppa, silo_state.set_package_version_list)
         silo_state.set_published()
     except CITrainError as err:

=== modified file 'citrain/recipes/merge.py'
--- citrain/recipes/merge.py	2015-10-21 03:02:09 +0000
+++ citrain/recipes/merge.py	2015-10-22 23:24:39 +0000
@@ -100,7 +100,11 @@
                    merge proposal URLs needed to build that package.
     """
     all_mps = {}
-    merges = None
+
+    def __init__(self, *args):
+        """Select only the merges we need from the all_mps dict."""
+        super().__init__(*args)
+        self.merges = Merge.all_mps.get(self.name, [])
 
     def enforce_merge_states(self, acceptable):
         """Block when merges have bad states."""
@@ -119,10 +123,9 @@
     def validate_phase(self):
         """Sanity check merges."""
         super().validate_phase()
-        merges = self.merges = Merge.all_mps[self.name]
         self.enforce_merge_states(BUILDABLE_STATES)
         if not self.failures:
-            check_merge_targets(merges)
+            check_merge_targets(self.merges)
 
     def collect_phase(self):
         """Commit merges locally.
@@ -235,27 +238,35 @@
             authors[BOT_DEBFULLNAME].append('New rebuild forced.')
         return authors
 
+    def expose_phase(self):
+        """Push branches to Launchpad so that they can be inspected."""
+        project = self.merges[0].target_branch.project.name
+        branch = '-'.join([
+            self.name,
+            self.dest.distribution.name,
+            self.series.name,
+            self.silo_ppa.name])
+        dest = 'lp:~/{}/{}'.format(project, branch)
+        self.push_to_branch(dest, overwrite=True)
+
     def unapproved_phase(self):
         """Block publication of merges not in approved states."""
-        self.merges = Merge.all_mps[self.name]
         if env.ALLOW_UNAPPROVED != 'true':
             self.enforce_merge_states(PUBLISHABLE_STATES)
 
     def unbuilt_phase(self):
         """Block publication of merges that have new commits."""
-        merges = Merge.all_mps[self.name]
-        if check_for_unbuilt_revids(self.name, merges):
+        if check_for_unbuilt_revids(self.name, self.merges):
             mark_packages_dirty(env.SILONAME, [self.name])
             self.failures.add(self.name + ' has new, unbuilt commits.')
 
     def merge_phase(self):
         """Merge destination trunk back into local branch, if necessary."""
         self.revert_branch()
-        target = self.target = Merge.all_mps[self.name][0].target_branch
+        target = self.merges[0].target_branch
         logging.info('Merging {} to {}.'.format(target.web_link, self.path))
         self.merge_branch(target.web_link, 'Resync trunk.', unchanged=False)
 
     def push_phase(self):
         """Push local branch to destination branch."""
-        logging.info('Pushing {} to {}.'.format(self.name, self.target))
-        self.push_to_branch(self.target.display_name)
+        self.push_to_branch(self.merges[0].target_branch.display_name)

=== modified file 'cupstream2distro/branchhandling.py'
--- cupstream2distro/branchhandling.py	2015-09-24 03:45:54 +0000
+++ cupstream2distro/branchhandling.py	2015-10-22 23:24:39 +0000
@@ -121,9 +121,6 @@
             raise BranchError('debian/changelog unreachable: {}'.format(err))
         return log.split()[0]
 
-    def __init__(self, path):
-        self.path = path
-
     def initialize_branch(self):
         """Create a new bzr branch in self.path."""
         log_call('bzr init .', cwd=self.path)
@@ -279,6 +276,7 @@
         :raises: BranchError if branch is not writable by us.
         """
         assert branch.startswith('lp:'), branch
+        logging.info('Pushing {} to {}.'.format(self.name, branch))
         command = ['bzr', 'push', branch]
         if overwrite:
             command.append('--overwrite')

=== modified file 'tests/unit/test_branchhandling.py'
--- tests/unit/test_branchhandling.py	2015-09-24 05:23:10 +0000
+++ tests/unit/test_branchhandling.py	2015-10-22 23:24:39 +0000
@@ -33,7 +33,9 @@
     """Test that we can handle a Branch!"""
     def setUp(self):
         super().setUp()
-        self.branch = Branch(self.tempdir)
+        self.branch = Branch()
+        self.branch.path = self.tempdir
+        self.branch.name = 'branchy'
 
     @patch('cupstream2distro.branchhandling.call')
     def test_get_package_name_from_branch(self, call_mock):

=== modified file 'tests/unit/test_recipe_merge.py'
--- tests/unit/test_recipe_merge.py	2015-10-21 03:02:09 +0000
+++ tests/unit/test_recipe_merge.py	2015-10-22 23:24:39 +0000
@@ -61,9 +61,13 @@
         super().setUp()
         self.source = Mock()
         self.dest = Mock()
+        self.dest.distribution.name = 'testbuntu'
         self.series = Mock()
+        self.series.name = 'xenial'
         self.ppa = Mock()
+        self.ppa.name = 'super-testy-fun-ppa'
         Merge.failures.clear()
+        Merge.all_mps.clear()
 
     def order_shuffler(self, expected, shuffles):
         """Shuffle and re-sort values, then compare against expectations."""
@@ -179,13 +183,11 @@
         """Ensure that validate phase blocks bad merges."""
         merge = Merge('bar', self.series, self.dest, self.ppa)
         for case in ('Work in progress', 'Needs review', 'Approved'):
-            Merge.all_mps = dict(bar=[Mock(
-                queue_status=case, web_link='example.com')])
+            merge.merges = [Mock(queue_status=case, web_link='example.com')]
             merge.validate_phase()
         self.assertFalse(merge.failures)
         for case in ('Superceded', 'Rejected', 'Really broken'):
-            Merge.all_mps = dict(bar=[Mock(
-                queue_status=case, web_link='example.com')])
+            merge.merges = [Mock(queue_status=case, web_link='example.com')]
             merge.validate_phase()
         self.assertEqual(merge.failures, {'bar has merges in bad states.'})
         self.assertEqual(log_mock.mock_calls, [
@@ -382,19 +384,29 @@
             merge.collect_package_bugs.return_value, take_whole_commit=False)
         self.assertEqual(authors, {'CI Train Bot': ['New rebuild forced.']})
 
+    def test_merge_expose_phase(self):
+        """Expose the locally merged branches in launchpad."""
+        project = Mock()
+        project.name = 'spilly'
+        merge = Merge('spill', self.series, self.dest, self.ppa)
+        merge.merges = [Mock(target_branch=Mock(project=project))]
+        merge.push_to_branch = Mock()
+        merge.expose_phase()
+        merge.push_to_branch.assert_called_once_with(
+            'lp:~/spilly/spill-testbuntu-xenial-super-testy-fun-ppa',
+            overwrite=True)
+
     @patch('citrain.recipes.merge.logging')
     def test_merge_unapproved_phase(self, log_mock):
         """Reject unapproved merges."""
         env.ALLOW_UNAPPROVED = 'false'
         merge = Merge('grill', self.series, self.dest, self.ppa)
         for case in ('Approved', 'Merged'):
-            Merge.all_mps = dict(grill=[Mock(
-                queue_status=case, web_link='example.com')])
+            merge.merges = [Mock(queue_status=case, web_link='example.com')]
             merge.unapproved_phase()
         self.assertFalse(merge.failures)
         for case in ('Superceded', 'Work in progress', 'Needs review'):
-            Merge.all_mps = dict(grill=[Mock(
-                queue_status=case, web_link='example.com')])
+            merge.merges = [Mock(queue_status=case, web_link='example.com')]
             merge.unapproved_phase()
         self.assertEqual(merge.failures, {'grill has merges in bad states.'})
         self.assertEqual(log_mock.mock_calls, [
@@ -414,8 +426,7 @@
         merge = Merge('grill', self.series, self.dest, self.ppa)
         merge.unapproved_phase()
         for case in ('Superceded', 'Work in progress', 'Needs review'):
-            Merge.all_mps = dict(grill=[Mock(
-                queue_status=case, web_link='example.com')])
+            merge.merges = [Mock(queue_status=case, web_link='example.com')]
             merge.unapproved_phase()
         self.assertFalse(merge.failures)
 
@@ -462,19 +473,19 @@
     @patch('citrain.recipes.merge.Merge.push_to_branch', Mock())
     def test_merge_push_phase(self):
         """Push branches to target."""
-        Merge.all_mps = {'foo-pack': ['a', 'b']}
+        mp = lambda name: Mock(target_branch=Mock(display_name=name))
+        Merge.all_mps = {'foo-pack': [mp('a'), mp('b')]}
         merge = Merge('foo-pack', self.series, self.dest, self.ppa)
-        merge.target = Mock()
         merge.push_phase()
-        merge.push_to_branch.assert_called_once_with(merge.target.display_name)
+        merge.push_to_branch.assert_called_once_with('a')
 
     @patch('citrain.recipes.merge.Merge.push_to_branch', Mock())
     def test_merge_push_phase_failed(self):
         """Push branches to target."""
-        Merge.all_mps = {'foo-pack': ['a', 'b']}
+        mp = lambda name: Mock(target_branch=Mock(display_name=name))
+        Merge.all_mps = {'foo-pack': [mp('a'), mp('b')]}
         merge = Merge('foo-pack', self.series, self.dest, self.ppa)
         merge.push_to_branch.side_effect = BranchError('Failed pushing')
-        merge.target = Mock()
         with self.assertRaisesRegexp(BranchError, 'Failed pushing'):
             merge.push_phase()
-        merge.push_to_branch.assert_called_once_with(merge.target.display_name)
+        merge.push_to_branch.assert_called_once_with('a')

=== modified file 'tests/unit/test_script_build.py'
--- tests/unit/test_script_build.py	2015-10-21 01:21:43 +0000
+++ tests/unit/test_script_build.py	2015-10-22 23:24:39 +0000
@@ -20,10 +20,6 @@
 
 from tests.unit import CITrainScriptTestCase
 
-from cupstream2distro.utils import suppress
-from cupstream2distro.packagemanager import Package
-from cupstream2distro.branchhandling import Branch
-from cupstream2distro.archive import Archive
 from cupstream2distro.errors import BuildError
 
 # Unused import necessary for code coverage reporting
@@ -31,19 +27,6 @@
 build_module
 
 
-def fake_factory():
-    """Generate a class that has mocked methods but is not itself a mock."""
-    class Fake(object):
-        """Dummy object for attaching mocked methods to."""
-        pass
-    for cls in (Branch, Package, Archive):
-        for name in cls.__dict__:
-            if '__' not in name:
-                with suppress(TypeError, AttributeError):
-                    setattr(Fake, name, Mock())
-    return Fake
-
-
 class BuildTestCase(CITrainScriptTestCase):
     """Test CI Train Build script."""
     scriptname = 'build.py'
@@ -144,7 +127,7 @@
         bman.assert_called_once_with(silo_state)
         self.assertEqual(bman.return_value.mock_calls, [
             call.do('validate'),
-            call.do('clean', 'collect', 'build', 'upload'),
+            call.do('clean', 'collect', 'build', 'upload', 'expose'),
         ])
         self.assertEqual(self.script.Manager.return_value.mock_calls, [
             call.do('watch', 'diff'),

=== modified file 'tests/unit/test_script_publisher.py'
--- tests/unit/test_script_publisher.py	2015-10-22 18:56:16 +0000
+++ tests/unit/test_script_publisher.py	2015-10-22 23:24:39 +0000
@@ -23,29 +23,12 @@
 from tests.unit import CITrainScriptTestCase
 
 from cupstream2distro.utils import env
-from cupstream2distro.errors import PublishError, BranchError, NoStatusError
+from cupstream2distro.errors import PublishError, NoStatusError
 
 
 N = '\n'
 
 
-class MergeFake(object):
-    """Simulate a launchpadlib merge object."""
-    source_branch = Mock(
-        revision_count=500,
-        last_scanned_id='[e-mail]-20140922102807-ld3vw6gt3dxsol2g')
-    target_branch = Mock()
-
-    def __init__(self, status='Approved', name=None):
-        """Set up some fake values."""
-        self.queue_status = status
-        self.web_link = 'example.com/1234'
-        self.target_branch.project.name = 'fooproject'
-        if name:
-            self.self_link = 'api.lp.net/' + name
-            self.web_link = 'lp.net/' + name
-
-
 class PublisherTestCase(CITrainScriptTestCase):
     """Test the CI Train Publisher."""
     scriptname = 'publisher.py'
@@ -71,42 +54,6 @@
         self.assertEqual(env.DISTRO, 'ubuntu-rtm')
         self.assertEqual(env.SERIES, '14.09')
 
-    def test_push_branches_success(self):
-        """Ensure that push_branches() can succeed."""
-        env.DISTRO = 'ubuntu'
-        env.SERIES = 'vivid'
-        mps = {
-            'foo': [MergeFake(name='a'), MergeFake(name='b')],
-            'bar': [MergeFake(name='c'), MergeFake(name='d')],
-        }
-        self.script.push_branches(mps)
-        branch = self.script.Branch.return_value
-        self.assertEqual(
-            branch.push_to_branch.mock_calls, [
-                call('lp:~/fooproject/bar-ubuntu-vivid-proposed',
-                     overwrite=True),
-                call('lp:~/fooproject/foo-ubuntu-vivid-proposed',
-                     overwrite=True),
-            ])
-
-    def test_push_branches_failure(self):
-        """Ensure that push_branches() handles failure."""
-        env.DISTRO = 'ubuntu-rtm'
-        env.SERIES = '14.09'
-        mps = {
-            'foo': [MergeFake(name='a'), MergeFake(name='b')],
-            'bar': [MergeFake(name='c'), MergeFake(name='d')],
-        }
-        branch = self.script.Branch.return_value
-        branch.push_to_branch.side_effect = BranchError('Failed to push')
-        with self.assertRaisesRegexp(BranchError, 'Failed to push'):
-            self.script.push_branches(mps)
-        self.assertEqual(
-            branch.push_to_branch.mock_calls, [
-                call('lp:~/fooproject/bar-ubuntu-rtm-14.09-proposed',
-                     overwrite=True),
-            ])
-
     def test_find_ppa_sources(self):
         """Use launchpadlib to correctly get the sources from a PPA."""
         ppa = Mock()
@@ -298,7 +245,6 @@
         """Ensure enough of the publisher is mocked out for testing main()."""
         self.script.Manager = Mock()
         self.script.get_distro_and_series = Mock()
-        self.script.push_branches = Mock()
         self.script.initiate_publication = Mock()
         self.script.SiloState.return_value.series = \
             'https://api.launchpad.net/devel/ubuntu/vivid'
@@ -316,7 +262,6 @@
             call('dest_version_check', 'unapproved', 'unbuilt'),
         ])
         self.assertEqual(state.status, 'Publishing.')
-        self.script.push_branches.assert_called_once_with(state.mps)
         self.script.initiate_publication.assert_called_once_with(
             dest, ppa, state.set_package_version_list)
         state.set_published.assert_called_once_with()
@@ -327,11 +272,11 @@
         """publisher.main() should handle errors."""
         self.mock_main()
         state = self.script.SiloState()
-        self.script.push_branches = Mock(side_effect=PublishError('Whoops'))
+        self.script.initiate_publication = Mock(
+            side_effect=PublishError('Whoops'))
         self.assertEqual(self.script.main(), 1)
         self.assertEqual(state.status, "Publish failed: Whoops")
-        self.script.push_branches.assert_called_once_with(state.mps)
-        self.assertEqual(self.script.initiate_publication.call_count, 0)
+        self.assertEqual(state.set_published.call_count, 0)
         self.script.logging.error.assert_called_once_with('Whoops')
         self.assertEqual(state.save_config.mock_calls, [call(), call()])
         self.assertEqual(state.mark_others_dirty.mock_calls, [])

=== modified file 'tests/unit/test_script_revert.py'
--- tests/unit/test_script_revert.py	2015-09-23 03:43:12 +0000
+++ tests/unit/test_script_revert.py	2015-10-22 23:24:39 +0000
@@ -20,7 +20,6 @@
 from mock import Mock, call
 
 from tests.unit import CITrainScriptTestCase
-from tests.unit.test_script_build import fake_factory
 from cupstream2distro.errors import RevertError
 
 # Unused import necessary for code coverage reporting
@@ -34,8 +33,7 @@
 
     def setUp(self):
         super().setUp()
-        self.script.Revert.__bases__ = (fake_factory(),)
-        self.revert = self.script.Revert()
+        self.revert = self.script.Revert(Mock(), Mock(), Mock(), Mock())
         self.revert.name = 'regress'
         self.revert.series = Mock(version='15.04')
         self.revert.dest = Mock()
@@ -53,6 +51,7 @@
         revert.delete_old_dsc = Mock()
         revert.rename_orig_tarball = Mock()
         revert.initialize_branch = Mock()
+        revert.get_from_archive = Mock()
         revert.collect_phase()
         self.script.sort_by_date.assert_called_once_with(
             revert.dest, source_name='regress',
@@ -86,6 +85,7 @@
         revert.delete_old_dsc = Mock()
         revert.rename_orig_tarball = Mock()
         revert.initialize_branch = Mock()
+        revert.get_from_archive = Mock()
         revert.collect_phase()
         self.script.sort_by_date.assert_called_once_with(
             revert.dest, source_name='regress',