Merge lp:~mew/charm-helpers/fetchallthethings into lp:charm-helpers

Proposed by Matthew Wedgwood
Status: Merged
Merged at revision: 48
Proposed branch: lp:~mew/charm-helpers/fetchallthethings
Merge into: lp:charm-helpers
Diff against target: 150 lines (+41/-15)
4 files modified
charmhelpers/fetch/archiveurl.py (+12/-7)
charmhelpers/payload/archive.py (+4/-3)
tests/fetch/test_archiveurl.py (+17/-2)
tests/payload/test_archive.py (+8/-3)
To merge this branch: bzr merge lp:~mew/charm-helpers/fetchallthethings
Reviewer Review Type Date Requested Status
Liam Young (community) Approve
Matthew Wedgwood (community) Needs Resubmitting
Review via email: mp+169552@code.launchpad.net

Commit message

Extensible support for retrieving software from external sources. Includes support for tar.gz, tar.bz2, and zip archives via http, https, or ftp.

Description of the change

This branch adds a fetch system to charm-helpers, allowing authors to easily support retrieval of external code from a number of different sources. In turn, they can allow their users to decide which sources they trust and how external code is retrieved.

Only one source handler (for tar.gz, tar.bz2, and zip archives via http, https, or ftp) is implemented in this branch. The BaseFetchHandler class allows additional handlers to be implemented easily.

As a peripheral change, 'make lint' no longer checks for line lengths in code.

To post a comment you must log in.
Revision history for this message
Liam Young (gnuoy) wrote :

I had a few issues when giving this a test run:

1) s/os.path.environ.get/os.environ.get/
2) install_remote expects a directory to be passed back by handler.install but the extract* classes don't return that in the archive class
3) For a tar ball the open method needs to be called on the tar ball and the extract all method is named differently i.e. This works for zip but not tar:
    archive = archive_class(archive_name)
    archive.extract_all(destpath)
This works for tar but not zip:
    archive = archive_class.open(archive_name)
    archive.extractall(destpath)
4) In the archive plugin the source archive needs to be passed when calling extract, i.e. it should be extract(url_parts.path, dest_file) not extract(dest_file)

review: Needs Fixing
26. By Adam Gandelman

[james-page] Import fixes for charmhelpers.contrib.hahelpers.

27. By James Page

[stub] Ensure that hookenv.relations also returns data that has been set by the local unit.

28. By Matthew Wedgwood

[michael.nelson] Add declarative machine state support using saltstack

29. By Matthew Wedgwood

[trivial] lint error cleanups

30. By Matthew Wedgwood

[michael.nelson] Don't use salt-minion (which installs and runs salt-minion daemon).

31. By Matthew Wedgwood

[james-page] Various refactoring + additions to hookenv

1) cached decorator

Allows caching of function + args to avoid repeated calls out the cli
to juju commands.

2) Normalizing missing unit/config/relation data to None

relation_get('missing') == None
config('missing') == None
unit_get('missing') == None

3) Normalized use of the Serializable object a bit

And added a unit test to cover missing attribute lookups

Question: maybe this should actually return 'None' rather than throwing
an exception inline with 2).

4) Tweaks some tests to be closer to juju behaviour with json

5) Tidied misc pep8/pylint warnings.

32. By Matthew Wedgwood

[james-page] Refactoring of service control code in host helper

1) Use 'service' command for all service control

Detecting upstart and init.d configuration files is overkill; this is
exactly what the 'service' command is design todo and it also deals with
saucy onwards where init.d and upstart configuration with the same
name might be installed.

'service' will always do the right thing

2) Added restart and reload helpers

reload detects an error (say the service is not running) and will fallback
to restart if so.

This is inline with the openstack charm helpers code.

33. By James Page

[stub] Misc fixes around use of Serializable including:

1) Make Serializable pickleable

2) Ensure string results are usable strings

34. By James Page

[gandelman-a]

Fixup broken test to ensure that services are only restarted once.

host.core.restart_on_change(): Ensure restarts happen as described in map.

35. By James Page

[gandelman-a]

This adds some small linux-specific utility helpers useful when working with block devices and storage. They are mostly derived from the Openstack helpers but renamed and tests added.

36. By Liam Young

[jamespage]
Redux of configure_source code in fetch helper

1) Fixed up a number of bugs and issues in the original code

2) Implement correct cloud: handling

3) Added tests for everything.

Revision history for this message
Matthew Wedgwood (mew) wrote :

I've fixed all of these issues, except (4). The command argument "dest_file" refers to the downloaded file, not the destination for extraction. I've renamed that to "dld_file" to make that a bit clearer.

I've also rearranged a bit, moving the archive handling code to the payload.archive module. fetch.archive.UrlArchiveFetchHandler is now renamed to fetch.archiveurl.ArchiveUrlFetchHandler.

review: Needs Resubmitting
Revision history for this message
Liam Young (gnuoy) wrote :

Theres a text conflict in charmhelpers/fetch/__init__.py

review: Needs Fixing
37. By Matthew Wedgwood

merge from trunk

Revision history for this message
Matthew Wedgwood (mew) wrote :

Conflict is resolved.

review: Needs Resubmitting
Revision history for this message
Liam Young (gnuoy) wrote :

It looks like ArchiveUrlFetchHandler is incorrectly reporting that is can't handle uncompressed tar files:

$ ls /tmp/testfetch/pay*
/tmp/testfetch/payload2.tar.gz /tmp/testfetch/payload.tar
$ python
Python 2.7.4 (default, Apr 19 2013, 18:32:33)
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import charmhelpers.fetch.archiveurl
>>> fh = charmhelpers.fetch.archiveurl.ArchiveUrlFetchHandler()
>>> fh.can_handle('file:///tmp/testfetch/payload.tar')
False
>>> fh.can_handle('file:///tmp/testfetch/payload2.tar.gz')
True
>>> quit()

Also, I think ArchiveUrlFetchHandlerTest should test a file uri that doesn't contain a hostname i.e. of the format file:///path/to/tarfile

review: Needs Fixing
38. By Matthew Wedgwood

Add support for uncompressed tar archives

Revision history for this message
Liam Young (gnuoy) wrote :

Deploying a tar file seems to fail:

root@liam-lxc-charmhelper-0:/var/lib/juju/units/charmhelper-0/charm/lib/charmhelper# mkdir /var/lib/juju/units/charmhelper-0/charm/fetched
root@liam-lxc-charmhelper-0:/var/lib/juju/units/charmhelper-0/charm/lib/charmhelper# python
Python 2.7.3 (default, Apr 10 2013, 05:46:21)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import charmhelpers.fetch.archiveurl
>>> fh = charmhelpers.fetch.archiveurl.ArchiveUrlFetchHandler()
>>> fh.can_handle('file:///tmp/payload.tar')
True
>>> fh.install('file:///tmp/payload.tar')
Unit name is not defined
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "charmhelpers/fetch/archiveurl.py", line 43, in install
    return extract(dld_file)
  File "charmhelpers/payload/archive.py", line 41, in extract
    get_archive_handler(archive_name)(archive_name, destpath)
  File "charmhelpers/payload/archive.py", line 49, in extract_tarfile
    archive = tarfile.open(archive_name)
  File "/usr/lib/python2.7/tarfile.py", line 1660, in open
    return func(name, "r", fileobj, **kwargs)
  File "/usr/lib/python2.7/tarfile.py", line 1722, in gzopen
    fileobj = bltn_open(name, mode + "b")
IOError: [Errno 21] Is a directory: '/var/lib/juju/units/charmhelper-0/charm/fetched/payload.tar'
>>>

review: Needs Fixing
39. By Matthew Wedgwood

fix stupid confusing bugs in downloads and archive extraction. update tests to catch them

Revision history for this message
Matthew Wedgwood (mew) wrote :

OK, I've fixed the bugs, updated the tests, and taken if for a spin. Sorry for crummy MP.

review: Needs Resubmitting
Revision history for this message
Liam Young (gnuoy) wrote :

Sorry to be a pita but it seems counter intuitive that the archives and fetched directory are both used during the process and the code creates the archives dir but fails if the fetched directory hasn't been precreated. Could the fetched directory be created on demand as well ?

review: Needs Fixing
40. By Matthew Wedgwood

Create 'fetched' directory if it does not exist

Revision history for this message
Matthew Wedgwood (mew) wrote :

Makes total sense. Thanks for the attention to detail.

review: Needs Resubmitting
Revision history for this message
Liam Young (gnuoy) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charmhelpers/fetch/archiveurl.py'
2--- charmhelpers/fetch/archiveurl.py 2013-06-26 22:14:29 +0000
3+++ charmhelpers/fetch/archiveurl.py 2013-07-10 18:59:30 +0000
4@@ -8,6 +8,7 @@
5 get_archive_handler,
6 extract,
7 )
8+from charmhelpers.core.host import mkdir
9
10
11 class ArchiveUrlFetchHandler(BaseFetchHandler):
12@@ -24,20 +25,24 @@
13 # propogate all exceptions
14 # URLError, OSError, etc
15 response = urllib2.urlopen(source)
16- with open(dest, 'w') as dest_file:
17- dest_file.write(response.read())
18+ try:
19+ with open(dest, 'w') as dest_file:
20+ dest_file.write(response.read())
21+ except Exception as e:
22+ if os.path.isfile(dest):
23+ os.unlink(dest)
24+ raise e
25
26 def install(self, source):
27 url_parts = self.parse_url(source)
28 dest_dir = os.path.join(os.environ.get('CHARM_DIR'), 'fetched')
29+ if not os.path.exists(dest_dir):
30+ mkdir(dest_dir, perms=0755)
31 dld_file = os.path.join(dest_dir, os.path.basename(url_parts.path))
32 try:
33 self.download(source, dld_file)
34 except urllib2.URLError as e:
35- return UnhandledSource(e.reason)
36+ raise UnhandledSource(e.reason)
37 except OSError as e:
38- return UnhandledSource(e.strerror)
39- finally:
40- if os.path.isfile(dld_file):
41- os.unlink(dld_file)
42+ raise UnhandledSource(e.strerror)
43 return extract(dld_file)
44
45=== modified file 'charmhelpers/payload/archive.py'
46--- charmhelpers/payload/archive.py 2013-06-26 22:14:29 +0000
47+++ charmhelpers/payload/archive.py 2013-07-10 18:59:30 +0000
48@@ -19,7 +19,7 @@
49 return extract_zipfile
50 else:
51 # look at the file name
52- for ext in ('.tar.gz', '.tgz', 'tar.bz2', '.tbz2', '.tbz'):
53+ for ext in ('.tar', '.tar.gz', '.tgz', 'tar.bz2', '.tbz2', '.tbz'):
54 if archive_name.endswith(ext):
55 return extract_tarfile
56 for ext in ('.zip', '.jar'):
57@@ -28,7 +28,8 @@
58
59
60 def archive_dest_default(archive_name):
61- return os.path.join(hookenv.charm_dir(), "archives", archive_name)
62+ archive_file = os.path.basename(archive_name)
63+ return os.path.join(hookenv.charm_dir(), "archives", archive_file)
64
65
66 def extract(archive_name, destpath=None):
67@@ -38,7 +39,7 @@
68 destpath = archive_dest_default(archive_name)
69 if not os.path.isdir(destpath):
70 host.mkdir(destpath)
71- get_archive_handler(archive_name)(archive_name, destpath)
72+ handler(archive_name, destpath)
73 return destpath
74 else:
75 raise ArchiveError("No handler for archive")
76
77=== modified file 'tests/fetch/test_archiveurl.py'
78--- tests/fetch/test_archiveurl.py 2013-06-26 22:14:29 +0000
79+++ tests/fetch/test_archiveurl.py 2013-07-10 18:59:30 +0000
80@@ -6,7 +6,11 @@
81 patch,
82 mock_open,
83 )
84-from charmhelpers.fetch import archiveurl
85+from charmhelpers.fetch import (
86+ archiveurl,
87+ UnhandledSource,
88+)
89+import urllib2
90
91
92 class ArchiveUrlFetchHandlerTest(TestCase):
93@@ -59,8 +63,9 @@
94 _open.assert_called_once_with("foo", 'w')
95 _open().write.assert_called_with("bar")
96
97+ @patch('charmhelpers.fetch.archiveurl.mkdir')
98 @patch('charmhelpers.fetch.archiveurl.extract')
99- def test_installs(self, _extract):
100+ def test_installs(self, _extract, _mkdir):
101 self.fh.download = MagicMock()
102
103 for url in self.valid_urls:
104@@ -72,3 +77,13 @@
105 self.fh.download.assert_called_with(url, dest)
106 _extract.assert_called_with(dest)
107 self.assertEqual(where, dest)
108+
109+ url = "http://www.example.com/archive.tar.gz"
110+
111+ self.fh.download.side_effect = urllib2.URLError('fail')
112+ with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
113+ self.assertRaises(UnhandledSource, self.fh.install, url)
114+
115+ self.fh.download.side_effect = OSError('fail')
116+ with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
117+ self.assertRaises(UnhandledSource, self.fh.install, url)
118
119=== modified file 'tests/payload/test_archive.py'
120--- tests/payload/test_archive.py 2013-06-27 15:59:39 +0000
121+++ tests/payload/test_archive.py 2013-07-10 18:59:30 +0000
122@@ -33,13 +33,15 @@
123 zip_archive_handler = archive.extract_zipfile
124 _isfile.return_value = False
125
126- for ext in ('tar.gz', 'tgz', 'tar.bz2', 'tbz2', 'tbz'):
127+ for ext in ('tar', 'tar.gz', 'tgz', 'tar.bz2', 'tbz2', 'tbz'):
128 handler = archive.get_archive_handler("somefile.{}".format(ext))
129- self.assertEqual(handler, tar_archive_handler)
130+ msg = "handler for extension: {}".format(ext)
131+ self.assertEqual(handler, tar_archive_handler, msg)
132
133 for ext in ('zip', 'jar'):
134 handler = archive.get_archive_handler("somefile.{}".format(ext))
135- self.assertEqual(handler, zip_archive_handler)
136+ msg = "handler for extension {}".format(ext)
137+ self.assertEqual(handler, zip_archive_handler, msg)
138
139 @patch('zipfile.is_zipfile')
140 @patch('tarfile.is_tarfile')
141@@ -66,6 +68,9 @@
142 thedir = archive.archive_dest_default("baz")
143 self.assertEqual(thedir, os.path.join("foo", "archives", "baz"))
144
145+ thedir = archive.archive_dest_default("baz/qux")
146+ self.assertEqual(thedir, os.path.join("foo", "archives", "qux"))
147+
148 def test_extracts_tarfile(self):
149 destdir = mkdtemp()
150 self.addCleanup(rmtree, destdir)

Subscribers

People subscribed via source and target branches