Charm Helpers

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

  1. py3-bzr
  2. Merge into devel
Proposed by Cory Johns
Status: Merged
Merged at revision: 507
Proposed branch: lp:~johnsca/charm-helpers/py3-bzr
Merge into: lp:charm-helpers
Diff against target: 385 lines (+91/-77)
7 files modified
charmhelpers/fetch/__init__.py (+1/-1)
charmhelpers/fetch/bzrurl.py (+15/-29)
charmhelpers/fetch/giturl.py (+18/-16)
test_requirements.txt (+0/-1)
tests/fetch/test_bzrurl.py (+24/-14)
tests/fetch/test_fetch.py (+2/-6)
tests/fetch/test_giturl.py (+31/-10)
To merge this branch: bzr merge lp:~johnsca/charm-helpers/py3-bzr
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Review via email: mp+280088@code.launchpad.net

Description of the change

Python 3 support by using CLI bzr instead of bzrlib

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

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
Revision history for this message
Cory Johns (johnsca) wrote :

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 updated
505. By Cory Johns

Merged upstream changes

506. By Cory Johns

Fixes from review

507. By Cory Johns

Fixed git & bzr install failure handling

Revision history for this message
Cory Johns (johnsca) wrote :

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.

Revision history for this message
Stuart Bishop (stub) wrote :

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 updated
508. By Cory Johns

Merged upstream changes

509. By Cory Johns

Converted giturl to use CLI and be idempotent

Revision history for this message
Cory Johns (johnsca) wrote :

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`?).

Revision history for this message
Stuart Bishop (stub) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
411 importlib.import_module(package),411 importlib.import_module(package),
412 classname)412 classname)
413 plugin_list.append(handler_class())413 plugin_list.append(handler_class())
414 except (ImportError, AttributeError):414 except NotImplementedError:
415 # Skip missing plugins so that they can be ommitted from415 # Skip missing plugins so that they can be ommitted from
416 # installation if desired416 # installation if desired
417 log("FetchHandler {} not found, skipping plugin".format(417 log("FetchHandler {} not found, skipping plugin".format(
418418
=== 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 @@
15# along with charm-helpers. If not, see <http://www.gnu.org/licenses/>.15# along with charm-helpers. If not, see <http://www.gnu.org/licenses/>.
1616
17import os17import os
18from subprocess import check_call
18from charmhelpers.fetch import (19from charmhelpers.fetch import (
19 BaseFetchHandler,20 BaseFetchHandler,
20 UnhandledSource21 UnhandledSource,
22 filter_installed_packages,
23 apt_install,
21)24)
22from charmhelpers.core.host import mkdir25from charmhelpers.core.host import mkdir
2326
24import six
25if six.PY3:
26 raise ImportError('bzrlib does not support Python3')
2727
28try:28if filter_installed_packages(['bzr']) != []:
29 from bzrlib.branch import Branch29 apt_install(['bzr'])
30 from bzrlib import bzrdir, workingtree, errors30 if filter_installed_packages(['bzr']) != []:
31except ImportError:31 raise NotImplementedError('Unable to install bzr')
32 from charmhelpers.fetch import apt_install
33 apt_install("python-bzrlib")
34 from bzrlib.branch import Branch
35 from bzrlib import bzrdir, workingtree, errors
3632
3733
38class BzrUrlFetchHandler(BaseFetchHandler):34class BzrUrlFetchHandler(BaseFetchHandler):
39 """Handler for bazaar branches via generic and lp URLs"""35 """Handler for bazaar branches via generic and lp URLs"""
40 def can_handle(self, source):36 def can_handle(self, source):
41 url_parts = self.parse_url(source)37 url_parts = self.parse_url(source)
42 if url_parts.scheme not in ('bzr+ssh', 'lp'):38 if url_parts.scheme not in ('bzr+ssh', 'lp', ''):
43 return False39 return False
40 elif not url_parts.scheme:
41 return os.path.exists(os.path.join(source, '.bzr'))
44 else:42 else:
45 return True43 return True
4644
47 def branch(self, source, dest):45 def branch(self, source, dest):
48 url_parts = self.parse_url(source)
49 # If we use lp:branchname scheme we need to load plugins
50 if not self.can_handle(source):46 if not self.can_handle(source):
51 raise UnhandledSource("Cannot handle {}".format(source))47 raise UnhandledSource("Cannot handle {}".format(source))
52 if url_parts.scheme == "lp":48 if os.path.exists(dest):
53 from bzrlib.plugin import load_plugins49 check_call(['bzr', 'pull', '--overwrite', '-d', dest, source])
54 load_plugins()50 else:
55 try:51 check_call(['bzr', 'branch', source, dest])
56 local_branch = bzrdir.BzrDir.create_branch_convenience(dest)
57 except errors.AlreadyControlDirError:
58 local_branch = Branch.open(dest)
59 try:
60 remote_branch = Branch.open(source)
61 remote_branch.push(local_branch)
62 tree = workingtree.WorkingTree.open(dest)
63 tree.update()
64 except Exception as e:
65 raise e
6652
67 def install(self, source, dest=None):53 def install(self, source, dest=None):
68 url_parts = self.parse_url(source)54 url_parts = self.parse_url(source)
6955
=== 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 @@
15# along with charm-helpers. If not, see <http://www.gnu.org/licenses/>.15# along with charm-helpers. If not, see <http://www.gnu.org/licenses/>.
1616
17import os17import os
18from subprocess import check_call
18from charmhelpers.fetch import (19from charmhelpers.fetch import (
19 BaseFetchHandler,20 BaseFetchHandler,
20 UnhandledSource21 UnhandledSource,
22 filter_installed_packages,
23 apt_install,
21)24)
22from charmhelpers.core.host import mkdir25from charmhelpers.core.host import mkdir
2326
24try:27if filter_installed_packages(['git']) != []:
25 from git import Repo28 apt_install(['git'])
26except ImportError:29 if filter_installed_packages(['git']) != []:
27 from charmhelpers.fetch import apt_install30 raise NotImplementedError('Unable to install git')
28 apt_install("python-git")
29 from git import Repo
30
31from git.exc import GitCommandError # noqa E402
3231
3332
34class GitUrlFetchHandler(BaseFetchHandler):33class GitUrlFetchHandler(BaseFetchHandler):
@@ -36,19 +35,24 @@
36 def can_handle(self, source):35 def can_handle(self, source):
37 url_parts = self.parse_url(source)36 url_parts = self.parse_url(source)
38 # TODO (mattyw) no support for ssh git@ yet37 # TODO (mattyw) no support for ssh git@ yet
39 if url_parts.scheme not in ('http', 'https', 'git'):38 if url_parts.scheme not in ('http', 'https', 'git', ''):
40 return False39 return False
40 elif not url_parts.scheme:
41 return os.path.exists(os.path.join(source, '.git'))
41 else:42 else:
42 return True43 return True
4344
44 def clone(self, source, dest, branch, depth=None):45 def clone(self, source, dest, branch="master", depth=None):
45 if not self.can_handle(source):46 if not self.can_handle(source):
46 raise UnhandledSource("Cannot handle {}".format(source))47 raise UnhandledSource("Cannot handle {}".format(source))
4748
49 if os.path.exists(dest):
50 cmd = ['git', '-C', dest, 'pull', source, branch]
51 else:
52 cmd = ['git', 'clone', source, dest, '--branch', branch]
48 if depth:53 if depth:
49 Repo.clone_from(source, dest, branch=branch, depth=depth)54 cmd.extend(['--depth', depth])
50 else:55 check_call(cmd)
51 Repo.clone_from(source, dest, branch=branch)
5256
53 def install(self, source, branch="master", dest=None, depth=None):57 def install(self, source, branch="master", dest=None, depth=None):
54 url_parts = self.parse_url(source)58 url_parts = self.parse_url(source)
@@ -62,8 +66,6 @@
62 mkdir(dest_dir, perms=0o755)66 mkdir(dest_dir, perms=0o755)
63 try:67 try:
64 self.clone(source, dest_dir, branch, depth)68 self.clone(source, dest_dir, branch, depth)
65 except GitCommandError as e:
66 raise UnhandledSource(e)
67 except OSError as e:69 except OSError as e:
68 raise UnhandledSource(e.strerror)70 raise UnhandledSource(e.strerror)
69 return dest_dir71 return dest_dir
7072
=== 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 @@
17netifaces==0.10 # trusty is 0.8, but using py3 compatible version for tests.17netifaces==0.10 # trusty is 0.8, but using py3 compatible version for tests.
18Jinja2==2.6 # precise18Jinja2==2.6 # precise
19six==1.1 # precise19six==1.1 # precise
20GitPython>=0.3.4 # trusty is ~0.3.2, but using py3 compatible version for tests.
2120
=== 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 @@
1import os1import os
2import shutil
3import subprocess
4import tempfile
2from testtools import TestCase5from testtools import TestCase
3from mock import (6from mock import (
4 MagicMock,7 MagicMock,
5 patch,8 patch,
6)9)
7import unittest
810
9import six11import six
10if six.PY3:12if six.PY3:
@@ -22,13 +24,10 @@
22 UnhandledSource = None24 UnhandledSource = None
2325
2426
25@unittest.skipIf(six.PY3, 'bzr does not support Python 3')
26class BzrUrlFetchHandlerTest(TestCase):27class BzrUrlFetchHandlerTest(TestCase):
2728
28 def setUp(self):29 def setUp(self):
29 super(BzrUrlFetchHandlerTest, self).setUp()30 super(BzrUrlFetchHandlerTest, self).setUp()
30 if six.PY3:
31 return
32 self.valid_urls = (31 self.valid_urls = (
33 "bzr+ssh://example.com/branch-name",32 "bzr+ssh://example.com/branch-name",
34 "bzr+ssh://example.com/branch-name/",33 "bzr+ssh://example.com/branch-name/",
@@ -55,7 +54,6 @@
55 )54 )
56 self.fh = bzrurl.BzrUrlFetchHandler()55 self.fh = bzrurl.BzrUrlFetchHandler()
5756
58 @unittest.skipIf(six.PY3, 'bzr does not support Python 3')
59 def test_handles_bzr_urls(self):57 def test_handles_bzr_urls(self):
60 for url in self.valid_urls:58 for url in self.valid_urls:
61 result = self.fh.can_handle(url)59 result = self.fh.can_handle(url)
@@ -64,26 +62,38 @@
64 result = self.fh.can_handle(url)62 result = self.fh.can_handle(url)
65 self.assertNotEqual(result, True, url)63 self.assertNotEqual(result, True, url)
6664
67 @unittest.skipIf(six.PY3, 'bzr does not support Python 3')65 @patch('charmhelpers.fetch.bzrurl.check_call')
68 @patch('bzrlib.workingtree.WorkingTree.open')66 def test_branch(self, check_call):
69 @patch('bzrlib.bzrdir.BzrDir.create_branch_convenience')
70 @patch('bzrlib.branch.Branch.open')
71 def test_branch(self, _open, _bzrdir, _tree_open):
72 dest_path = "/destination/path"67 dest_path = "/destination/path"
73 for url in self.valid_urls:68 for url in self.valid_urls:
74 self.fh.remote_branch = MagicMock()69 self.fh.remote_branch = MagicMock()
75 self.fh.load_plugins = MagicMock()70 self.fh.load_plugins = MagicMock()
76 self.fh.branch(url, dest_path)71 self.fh.branch(url, dest_path)
7772
78 _open.assert_called_with(url)73 check_call.assert_called_with(['bzr', 'branch', url, dest_path])
79 _bzrdir.assert_called_with(dest_path)
80 _tree_open.assert_called_with(dest_path)
8174
82 for url in self.invalid_urls:75 for url in self.invalid_urls:
83 with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):76 with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
84 self.assertRaises(UnhandledSource, self.fh.branch, url, dest_path)77 self.assertRaises(UnhandledSource, self.fh.branch, url, dest_path)
8578
86 @unittest.skipIf(six.PY3, 'bzr does not support Python 3')79 def test_branch_functional(self):
80 src = None
81 dst = None
82 try:
83 src = tempfile.mkdtemp()
84 subprocess.check_call(['bzr', 'init', src])
85 dst = tempfile.mkdtemp()
86 os.rmdir(dst)
87 self.fh.branch(src, dst)
88 assert os.path.exists(os.path.join(dst, '.bzr'))
89 self.fh.branch(src, dst) # idempotent
90 assert os.path.exists(os.path.join(dst, '.bzr'))
91 finally:
92 if src:
93 shutil.rmtree(src, ignore_errors=True)
94 if dst:
95 shutil.rmtree(dst, ignore_errors=True)
96
87 @patch('charmhelpers.fetch.bzrurl.mkdir')97 @patch('charmhelpers.fetch.bzrurl.mkdir')
88 def test_installs(self, _mkdir):98 def test_installs(self, _mkdir):
89 self.fh.branch = MagicMock()99 self.fh.branch = MagicMock()
90100
=== 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 @@
403 @patch('charmhelpers.fetch.importlib.import_module')403 @patch('charmhelpers.fetch.importlib.import_module')
404 def test_skips_and_logs_missing_plugins(self, import_, log_):404 def test_skips_and_logs_missing_plugins(self, import_, log_):
405 fetch_handlers = ['a.foo', 'b.foo', 'c.foo']405 fetch_handlers = ['a.foo', 'b.foo', 'c.foo']
406 import_.side_effect = (ImportError, AttributeError, MagicMock())406 import_.side_effect = (NotImplementedError, NotImplementedError, MagicMock())
407 plugins = fetch.plugins(fetch_handlers)407 plugins = fetch.plugins(fetch_handlers)
408408
409 self.assertEqual(1, len(plugins))409 self.assertEqual(1, len(plugins))
@@ -412,11 +412,7 @@
412 @patch('charmhelpers.fetch.log')412 @patch('charmhelpers.fetch.log')
413 def test_plugins_are_valid(self, log_):413 def test_plugins_are_valid(self, log_):
414 plugins = fetch.plugins()414 plugins = fetch.plugins()
415 if not six.PY3:415 self.assertEqual(len(fetch.FETCH_HANDLERS), len(plugins))
416 self.assertEqual(len(fetch.FETCH_HANDLERS), len(plugins))
417 else:
418 # No bzr libraries for Python3.
419 self.assertEqual(len(fetch.FETCH_HANDLERS) - 1, len(plugins))
420416
421417
422class BaseFetchHandlerTest(TestCase):418class BaseFetchHandlerTest(TestCase):
423419
=== 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 @@
1import os1import os
2import shutil
3import subprocess
4import tempfile
2from testtools import TestCase5from testtools import TestCase
3from mock import (6from mock import (
4 MagicMock,7 MagicMock,
5 patch,8 patch,
6)9)
7import unittest
810
9import six11import six
10if six.PY3:12if six.PY3:
@@ -12,6 +14,7 @@
12else:14else:
13 from urlparse import urlparse15 from urlparse import urlparse
1416
17from charmhelpers.core.host import chdir
15try:18try:
16 from charmhelpers.fetch import (19 from charmhelpers.fetch import (
17 giturl,20 giturl,
@@ -23,8 +26,6 @@
2326
2427
25class GitUrlFetchHandlerTest(TestCase):28class GitUrlFetchHandlerTest(TestCase):
26
27 @unittest.skipIf(six.PY3, 'git does not support Python 3')
28 def setUp(self):29 def setUp(self):
29 super(GitUrlFetchHandlerTest, self).setUp()30 super(GitUrlFetchHandlerTest, self).setUp()
30 self.valid_urls = (31 self.valid_urls = (
@@ -39,7 +40,6 @@
39 )40 )
40 self.fh = giturl.GitUrlFetchHandler()41 self.fh = giturl.GitUrlFetchHandler()
4142
42 @unittest.skipIf(six.PY3, 'git does not support Python 3')
43 def test_handles_git_urls(self):43 def test_handles_git_urls(self):
44 for url in self.valid_urls:44 for url in self.valid_urls:
45 result = self.fh.can_handle(url)45 result = self.fh.can_handle(url)
@@ -48,9 +48,8 @@
48 result = self.fh.can_handle(url)48 result = self.fh.can_handle(url)
49 self.assertNotEqual(result, True, url)49 self.assertNotEqual(result, True, url)
5050
51 @unittest.skipIf(six.PY3, 'git does not support Python 3')51 @patch.object(giturl, 'check_call')
52 @patch('git.Repo.clone_from')52 def test_clone(self, check_call):
53 def test_branch(self, _clone_from):
54 dest_path = "/destination/path"53 dest_path = "/destination/path"
55 branch = "master"54 branch = "master"
56 for url in self.valid_urls:55 for url in self.valid_urls:
@@ -58,7 +57,7 @@
58 self.fh.load_plugins = MagicMock()57 self.fh.load_plugins = MagicMock()
59 self.fh.clone(url, dest_path, branch, None)58 self.fh.clone(url, dest_path, branch, None)
6059
61 _clone_from.assert_called_with(url, dest_path, branch=branch)60 check_call.assert_called_with(['git', 'clone', url, dest_path, '--branch', branch])
6261
63 for url in self.invalid_urls:62 for url in self.invalid_urls:
64 with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):63 with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
@@ -66,7 +65,30 @@
66 dest_path, None,65 dest_path, None,
67 branch)66 branch)
6867
69 @unittest.skipIf(six.PY3, 'git does not support Python 3')68 def test_clone_functional(self):
69 src = None
70 dst = None
71 try:
72 src = tempfile.mkdtemp()
73 with chdir(src):
74 subprocess.check_call(['git', 'init'])
75 subprocess.check_call(['git', 'config', 'user.name', 'Joe'])
76 subprocess.check_call(['git', 'config', 'user.email', 'joe@test.com'])
77 subprocess.check_call(['touch', 'foo'])
78 subprocess.check_call(['git', 'add', 'foo'])
79 subprocess.check_call(['git', 'commit', '-m', 'test'])
80 dst = tempfile.mkdtemp()
81 os.rmdir(dst)
82 self.fh.clone(src, dst)
83 assert os.path.exists(os.path.join(dst, '.git'))
84 self.fh.clone(src, dst) # idempotent
85 assert os.path.exists(os.path.join(dst, '.git'))
86 finally:
87 if src:
88 shutil.rmtree(src, ignore_errors=True)
89 if dst:
90 shutil.rmtree(dst, ignore_errors=True)
91
70 @patch('charmhelpers.fetch.giturl.mkdir')92 @patch('charmhelpers.fetch.giturl.mkdir')
71 def test_installs(self, _mkdir):93 def test_installs(self, _mkdir):
72 self.fh.clone = MagicMock()94 self.fh.clone = MagicMock()
@@ -80,7 +102,6 @@
80 self.assertEqual(where, dest)102 self.assertEqual(where, dest)
81 _mkdir.assert_called_with(where, perms=0o755)103 _mkdir.assert_called_with(where, perms=0o755)
82104
83 @unittest.skipIf(six.PY3, 'git does not support Python 3')
84 @patch('charmhelpers.fetch.giturl.mkdir')105 @patch('charmhelpers.fetch.giturl.mkdir')
85 def test_installs_specified_dest(self, _mkdir):106 def test_installs_specified_dest(self, _mkdir):
86 self.fh.clone = MagicMock()107 self.fh.clone = MagicMock()

Subscribers

People subscribed via source and target branches