Merge lp:~johnsca/charm-helpers/py3-bzr into lp:charm-helpers

[Cory Johns]

Python 3 support by using CLI bzr instead of bzrlib

[Stuart Bishop (stub)]

Comments in line.

I think we need to keep the automagic package installation for now, as charm authors should not be responsible for bootstrapping the libraries that may be required by charmhelpers.fetch depending on the configuration set by the charm user. This will become a noop for reactive charms once it has a better way of bootstrapping architecture dependent packages (better than this ImportError hack at least).

The branch method needs to be idempotent. Simple fix described inline.

The existing tests seem rather barren. Again, alternatives described inline.

review: Needs Fixing

[Cory Johns (johnsca)]

You're right about it being an unexpected backwards-incompatible change. I just loathe the hidden install-on-demand behavior so much. :p

Thanks for the other comments. I will address them and update.

[lp:~johnsca/charm-helpers/py3-bzr]

505. By Cory Johns on 2015-12-10

Merged upstream changes

506. By Cory Johns on 2015-12-10

Fixes from review

507. By Cory Johns on 2015-12-10

Fixed git & bzr install failure handling

[Cory Johns (johnsca)]

I went with a simpler exists check for the idempotency even though it's not exactly equivalent to what you suggested. I trust it is still sufficient?

Also, I had to add support for local repos to get your suggested test case to work, but I think it's worth it, even if it's unlikely to be used in practice.

[Stuart Bishop (stub)]

All good, except for the git package bootstrap. Unlike 'apt-get install git', 'pip install GitPython' will not pull in the git(1) command line tool. It will also fail in environments without egress to pypi.

Humph. And python3-git doesn't exist yet (I just filed a bug to get the package built in Xenial).

So unless we want to say screw it and use command line git rather than a Python wrapper, the same as we are doing with bzr, then the best I can come up with is:
if six.PY2:
   apt_install(['python-git'])
   try:
       import git
   except ImportError:
       raise NotImplemetedError()
else:
   raise NotImplementedError()

ie. keep auto-install of the python-git package for Python2, and make it the charmers problem if they are using Python3. This is still an improvement, since at the moment Python3 charms do not have access to this API at all.

This can land if you revert the Python2 behaviour to the apt install, so giving it my conditional approval.

review: Approve

[lp:~johnsca/charm-helpers/py3-bzr]

508. By Cory Johns on 2015-12-11

Merged upstream changes

509. By Cory Johns on 2015-12-11

Converted giturl to use CLI and be idempotent

[Cory Johns (johnsca)]

I went ahead and converted git to use the CLI. Makes it cleaner and more consistent. I also fixed the idempotency issue for git. Previously, it would fail if called twice; now it works somewhat similarly to the bzr (though git doesn't have an analogue to --overwrite; maybe I should have just `rm -rf dest`?).

[Stuart Bishop (stub)]

All looks good to me.

If you have messed up your branches enough that the --overwrite is actually needed, there is some other problem. I just put it in my example for robustness. I wouldn't do the rm -rf, as that would suck if the source branch is huge.

review: Approve

=== modified file 'charmhelpers/fetch/__init__.py'
--- charmhelpers/fetch/__init__.py	2015-10-26 09:47:10 +0000
+++ charmhelpers/fetch/__init__.py	2015-12-11 17:02:29 +0000
@@ -411,7 +411,7 @@
                 importlib.import_module(package),
                 classname)
             plugin_list.append(handler_class())
-        except (ImportError, AttributeError):
+        except NotImplementedError:
             # Skip missing plugins so that they can be ommitted from
             # installation if desired
             log("FetchHandler {} not found, skipping plugin".format(

=== modified file 'charmhelpers/fetch/bzrurl.py'
--- charmhelpers/fetch/bzrurl.py	2015-11-22 21:31:56 +0000
+++ charmhelpers/fetch/bzrurl.py	2015-12-11 17:02:29 +0000
@@ -15,54 +15,40 @@
 # along with charm-helpers.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+from subprocess import check_call
 from charmhelpers.fetch import (
     BaseFetchHandler,
-    UnhandledSource
+    UnhandledSource,
+    filter_installed_packages,
+    apt_install,
 )
 from charmhelpers.core.host import mkdir
 
-import six
-if six.PY3:
-    raise ImportError('bzrlib does not support Python3')
 
-try:
-    from bzrlib.branch import Branch
-    from bzrlib import bzrdir, workingtree, errors
-except ImportError:
-    from charmhelpers.fetch import apt_install
-    apt_install("python-bzrlib")
-    from bzrlib.branch import Branch
-    from bzrlib import bzrdir, workingtree, errors
+if filter_installed_packages(['bzr']) != []:
+    apt_install(['bzr'])
+    if filter_installed_packages(['bzr']) != []:
+        raise NotImplementedError('Unable to install bzr')
 
 
 class BzrUrlFetchHandler(BaseFetchHandler):
     """Handler for bazaar branches via generic and lp URLs"""
     def can_handle(self, source):
         url_parts = self.parse_url(source)
-        if url_parts.scheme not in ('bzr+ssh', 'lp'):
+        if url_parts.scheme not in ('bzr+ssh', 'lp', ''):
             return False
+        elif not url_parts.scheme:
+            return os.path.exists(os.path.join(source, '.bzr'))
         else:
             return True
 
     def branch(self, source, dest):
-        url_parts = self.parse_url(source)
-        # If we use lp:branchname scheme we need to load plugins
         if not self.can_handle(source):
             raise UnhandledSource("Cannot handle {}".format(source))
-        if url_parts.scheme == "lp":
-            from bzrlib.plugin import load_plugins
-            load_plugins()
-        try:
-            local_branch = bzrdir.BzrDir.create_branch_convenience(dest)
-        except errors.AlreadyControlDirError:
-            local_branch = Branch.open(dest)
-        try:
-            remote_branch = Branch.open(source)
-            remote_branch.push(local_branch)
-            tree = workingtree.WorkingTree.open(dest)
-            tree.update()
-        except Exception as e:
-            raise e
+        if os.path.exists(dest):
+            check_call(['bzr', 'pull', '--overwrite', '-d', dest, source])
+        else:
+            check_call(['bzr', 'branch', source, dest])
 
     def install(self, source, dest=None):
         url_parts = self.parse_url(source)

=== modified file 'charmhelpers/fetch/giturl.py'
--- charmhelpers/fetch/giturl.py	2015-12-07 20:45:58 +0000
+++ charmhelpers/fetch/giturl.py	2015-12-11 17:02:29 +0000
@@ -15,20 +15,19 @@
 # along with charm-helpers.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+from subprocess import check_call
 from charmhelpers.fetch import (
     BaseFetchHandler,
-    UnhandledSource
+    UnhandledSource,
+    filter_installed_packages,
+    apt_install,
 )
 from charmhelpers.core.host import mkdir
 
-try:
-    from git import Repo
-except ImportError:
-    from charmhelpers.fetch import apt_install
-    apt_install("python-git")
-    from git import Repo
-
-from git.exc import GitCommandError  # noqa E402
+if filter_installed_packages(['git']) != []:
+    apt_install(['git'])
+    if filter_installed_packages(['git']) != []:
+        raise NotImplementedError('Unable to install git')
 
 
 class GitUrlFetchHandler(BaseFetchHandler):
@@ -36,19 +35,24 @@
     def can_handle(self, source):
         url_parts = self.parse_url(source)
         # TODO (mattyw) no support for ssh git@ yet
-        if url_parts.scheme not in ('http', 'https', 'git'):
+        if url_parts.scheme not in ('http', 'https', 'git', ''):
             return False
+        elif not url_parts.scheme:
+            return os.path.exists(os.path.join(source, '.git'))
         else:
             return True
 
-    def clone(self, source, dest, branch, depth=None):
+    def clone(self, source, dest, branch="master", depth=None):
         if not self.can_handle(source):
             raise UnhandledSource("Cannot handle {}".format(source))
 
+        if os.path.exists(dest):
+            cmd = ['git', '-C', dest, 'pull', source, branch]
+        else:
+            cmd = ['git', 'clone', source, dest, '--branch', branch]
         if depth:
-            Repo.clone_from(source, dest, branch=branch, depth=depth)
-        else:
-            Repo.clone_from(source, dest, branch=branch)
+            cmd.extend(['--depth', depth])
+        check_call(cmd)
 
     def install(self, source, branch="master", dest=None, depth=None):
         url_parts = self.parse_url(source)
@@ -62,8 +66,6 @@
             mkdir(dest_dir, perms=0o755)
         try:
             self.clone(source, dest_dir, branch, depth)
-        except GitCommandError as e:
-            raise UnhandledSource(e)
         except OSError as e:
             raise UnhandledSource(e.strerror)
         return dest_dir

=== modified file 'test_requirements.txt'
--- test_requirements.txt	2015-12-08 05:07:07 +0000
+++ test_requirements.txt	2015-12-11 17:02:29 +0000
@@ -17,4 +17,3 @@
 netifaces==0.10    # trusty is 0.8, but using py3 compatible version for tests.
 Jinja2==2.6        # precise
 six==1.1           # precise
-GitPython>=0.3.4   # trusty is ~0.3.2, but using py3 compatible version for tests.

=== modified file 'tests/fetch/test_bzrurl.py'
--- tests/fetch/test_bzrurl.py	2015-01-14 14:48:12 +0000
+++ tests/fetch/test_bzrurl.py	2015-12-11 17:02:29 +0000
@@ -1,10 +1,12 @@
 import os
+import shutil
+import subprocess
+import tempfile
 from testtools import TestCase
 from mock import (
     MagicMock,
     patch,
 )
-import unittest
 
 import six
 if six.PY3:
@@ -22,13 +24,10 @@
     UnhandledSource = None
 
 
[e-mail](six.PY3, 'bzr does not support Python 3')
 class BzrUrlFetchHandlerTest(TestCase):
 
     def setUp(self):
         super(BzrUrlFetchHandlerTest, self).setUp()
-        if six.PY3:
-            return
         self.valid_urls = (
             "bzr+ssh://example.com/branch-name",
             "bzr+ssh://example.com/branch-name/",
@@ -55,7 +54,6 @@
         )
         self.fh = bzrurl.BzrUrlFetchHandler()
 
-    @unittest.skipIf(six.PY3, 'bzr does not support Python 3')
     def test_handles_bzr_urls(self):
         for url in self.valid_urls:
             result = self.fh.can_handle(url)
@@ -64,26 +62,38 @@
             result = self.fh.can_handle(url)
             self.assertNotEqual(result, True, url)
 
-    @unittest.skipIf(six.PY3, 'bzr does not support Python 3')
-    @patch('bzrlib.workingtree.WorkingTree.open')
-    @patch('bzrlib.bzrdir.BzrDir.create_branch_convenience')
-    @patch('bzrlib.branch.Branch.open')
-    def test_branch(self, _open, _bzrdir, _tree_open):
+    @patch('charmhelpers.fetch.bzrurl.check_call')
+    def test_branch(self, check_call):
         dest_path = "/destination/path"
         for url in self.valid_urls:
             self.fh.remote_branch = MagicMock()
             self.fh.load_plugins = MagicMock()
             self.fh.branch(url, dest_path)
 
-            _open.assert_called_with(url)
-            _bzrdir.assert_called_with(dest_path)
-            _tree_open.assert_called_with(dest_path)
+            check_call.assert_called_with(['bzr', 'branch', url, dest_path])
 
         for url in self.invalid_urls:
             with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
                 self.assertRaises(UnhandledSource, self.fh.branch, url, dest_path)
 
-    @unittest.skipIf(six.PY3, 'bzr does not support Python 3')
+    def test_branch_functional(self):
+        src = None
+        dst = None
+        try:
+            src = tempfile.mkdtemp()
+            subprocess.check_call(['bzr', 'init', src])
+            dst = tempfile.mkdtemp()
+            os.rmdir(dst)
+            self.fh.branch(src, dst)
+            assert os.path.exists(os.path.join(dst, '.bzr'))
+            self.fh.branch(src, dst)  # idempotent
+            assert os.path.exists(os.path.join(dst, '.bzr'))
+        finally:
+            if src:
+                shutil.rmtree(src, ignore_errors=True)
+            if dst:
+                shutil.rmtree(dst, ignore_errors=True)
+
     @patch('charmhelpers.fetch.bzrurl.mkdir')
     def test_installs(self, _mkdir):
         self.fh.branch = MagicMock()

=== modified file 'tests/fetch/test_fetch.py'
--- tests/fetch/test_fetch.py	2015-12-08 05:07:07 +0000
+++ tests/fetch/test_fetch.py	2015-12-11 17:02:29 +0000
@@ -403,7 +403,7 @@
     @patch('charmhelpers.fetch.importlib.import_module')
     def test_skips_and_logs_missing_plugins(self, import_, log_):
         fetch_handlers = ['a.foo', 'b.foo', 'c.foo']
-        import_.side_effect = (ImportError, AttributeError, MagicMock())
+        import_.side_effect = (NotImplementedError, NotImplementedError, MagicMock())
         plugins = fetch.plugins(fetch_handlers)
 
         self.assertEqual(1, len(plugins))
@@ -412,11 +412,7 @@
     @patch('charmhelpers.fetch.log')
     def test_plugins_are_valid(self, log_):
         plugins = fetch.plugins()
-        if not six.PY3:
-            self.assertEqual(len(fetch.FETCH_HANDLERS), len(plugins))
-        else:
-            # No bzr libraries for Python3.
-            self.assertEqual(len(fetch.FETCH_HANDLERS) - 1, len(plugins))
+        self.assertEqual(len(fetch.FETCH_HANDLERS), len(plugins))
 
 
 class BaseFetchHandlerTest(TestCase):

=== modified file 'tests/fetch/test_giturl.py'
--- tests/fetch/test_giturl.py	2015-12-08 05:07:07 +0000
+++ tests/fetch/test_giturl.py	2015-12-11 17:02:29 +0000
@@ -1,10 +1,12 @@
 import os
+import shutil
+import subprocess
+import tempfile
 from testtools import TestCase
 from mock import (
     MagicMock,
     patch,
 )
-import unittest
 
 import six
 if six.PY3:
@@ -12,6 +14,7 @@
 else:
     from urlparse import urlparse
 
+from charmhelpers.core.host import chdir
 try:
     from charmhelpers.fetch import (
         giturl,
@@ -23,8 +26,6 @@
 
 
 class GitUrlFetchHandlerTest(TestCase):
-
-    @unittest.skipIf(six.PY3, 'git does not support Python 3')
     def setUp(self):
         super(GitUrlFetchHandlerTest, self).setUp()
         self.valid_urls = (
@@ -39,7 +40,6 @@
         )
         self.fh = giturl.GitUrlFetchHandler()
 
-    @unittest.skipIf(six.PY3, 'git does not support Python 3')
     def test_handles_git_urls(self):
         for url in self.valid_urls:
             result = self.fh.can_handle(url)
@@ -48,9 +48,8 @@
             result = self.fh.can_handle(url)
             self.assertNotEqual(result, True, url)
 
-    @unittest.skipIf(six.PY3, 'git does not support Python 3')
-    @patch('git.Repo.clone_from')
-    def test_branch(self, _clone_from):
+    @patch.object(giturl, 'check_call')
+    def test_clone(self, check_call):
         dest_path = "/destination/path"
         branch = "master"
         for url in self.valid_urls:
@@ -58,7 +57,7 @@
             self.fh.load_plugins = MagicMock()
             self.fh.clone(url, dest_path, branch, None)
 
-            _clone_from.assert_called_with(url, dest_path, branch=branch)
+            check_call.assert_called_with(['git', 'clone', url, dest_path, '--branch', branch])
 
         for url in self.invalid_urls:
             with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
@@ -66,7 +65,30 @@
                                   dest_path, None,
                                   branch)
 
-    @unittest.skipIf(six.PY3, 'git does not support Python 3')
+    def test_clone_functional(self):
+        src = None
+        dst = None
+        try:
+            src = tempfile.mkdtemp()
+            with chdir(src):
+                subprocess.check_call(['git', 'init'])
+                subprocess.check_call(['git', 'config', 'user.name', 'Joe'])
+                subprocess.check_call(['git', 'config', 'user.email', '[e-mail]'])
+                subprocess.check_call(['touch', 'foo'])
+                subprocess.check_call(['git', 'add', 'foo'])
+                subprocess.check_call(['git', 'commit', '-m', 'test'])
+            dst = tempfile.mkdtemp()
+            os.rmdir(dst)
+            self.fh.clone(src, dst)
+            assert os.path.exists(os.path.join(dst, '.git'))
+            self.fh.clone(src, dst)  # idempotent
+            assert os.path.exists(os.path.join(dst, '.git'))
+        finally:
+            if src:
+                shutil.rmtree(src, ignore_errors=True)
+            if dst:
+                shutil.rmtree(dst, ignore_errors=True)
+
     @patch('charmhelpers.fetch.giturl.mkdir')
     def test_installs(self, _mkdir):
         self.fh.clone = MagicMock()
@@ -80,7 +102,6 @@
             self.assertEqual(where, dest)
             _mkdir.assert_called_with(where, perms=0o755)
 
-    @unittest.skipIf(six.PY3, 'git does not support Python 3')
     @patch('charmhelpers.fetch.giturl.mkdir')
     def test_installs_specified_dest(self, _mkdir):
         self.fh.clone = MagicMock()