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

[Matthew Wedgwood]

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

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.

[Liam Young (gnuoy)]

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

[lp:~mew/charm-helpers/fetchallthethings]

26. By Adam Gandelman on 2013-06-17

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

27. By James Page on 2013-06-19

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

28. By Matthew Wedgwood on 2013-06-19

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

29. By Matthew Wedgwood on 2013-06-19

[trivial] lint error cleanups

30. By Matthew Wedgwood on 2013-06-19

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

31. By Matthew Wedgwood on 2013-06-19

[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 on 2013-06-21

[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 on 2013-06-23

[stub] Misc fixes around use of Serializable including:

1) Make Serializable pickleable

2) Ensure string results are usable strings

34. By James Page on 2013-06-24

[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 on 2013-06-25

[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 on 2013-06-26

[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.

[Matthew Wedgwood (mew)]

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

[Liam Young (gnuoy)]

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

review: Needs Fixing

[lp:~mew/charm-helpers/fetchallthethings]

37. By Matthew Wedgwood on 2013-06-27

merge from trunk

[Matthew Wedgwood (mew)]

Conflict is resolved.

review: Needs Resubmitting

[Liam Young (gnuoy)]

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

[lp:~mew/charm-helpers/fetchallthethings]

38. By Matthew Wedgwood on 2013-06-28

Add support for uncompressed tar archives

[Liam Young (gnuoy)]

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

[lp:~mew/charm-helpers/fetchallthethings]

39. By Matthew Wedgwood on 2013-07-03

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

[Matthew Wedgwood (mew)]

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

review: Needs Resubmitting

[Liam Young (gnuoy)]

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

[lp:~mew/charm-helpers/fetchallthethings]

40. By Matthew Wedgwood on 2013-07-10

Create 'fetched' directory if it does not exist

[Matthew Wedgwood (mew)]

Makes total sense. Thanks for the attention to detail.

review: Needs Resubmitting

[Liam Young (gnuoy)]

LGTM

review: Approve

=== modified file 'charmhelpers/fetch/archiveurl.py'
--- charmhelpers/fetch/archiveurl.py	2013-06-26 22:14:29 +0000
+++ charmhelpers/fetch/archiveurl.py	2013-07-10 18:59:30 +0000
@@ -8,6 +8,7 @@
     get_archive_handler,
     extract,
 )
+from charmhelpers.core.host import mkdir
 
 
 class ArchiveUrlFetchHandler(BaseFetchHandler):
@@ -24,20 +25,24 @@
         # propogate all exceptions
         # URLError, OSError, etc
         response = urllib2.urlopen(source)
-        with open(dest, 'w') as dest_file:
-            dest_file.write(response.read())
+        try:
+            with open(dest, 'w') as dest_file:
+                dest_file.write(response.read())
+        except Exception as e:
+            if os.path.isfile(dest):
+                os.unlink(dest)
+            raise e
 
     def install(self, source):
         url_parts = self.parse_url(source)
         dest_dir = os.path.join(os.environ.get('CHARM_DIR'), 'fetched')
+        if not os.path.exists(dest_dir):
+            mkdir(dest_dir, perms=0755)
         dld_file = os.path.join(dest_dir, os.path.basename(url_parts.path))
         try:
             self.download(source, dld_file)
         except urllib2.URLError as e:
-            return UnhandledSource(e.reason)
+            raise UnhandledSource(e.reason)
         except OSError as e:
-            return UnhandledSource(e.strerror)
-        finally:
-            if os.path.isfile(dld_file):
-                os.unlink(dld_file)
+            raise UnhandledSource(e.strerror)
         return extract(dld_file)
 
=== modified file 'charmhelpers/payload/archive.py'
--- charmhelpers/payload/archive.py	2013-06-26 22:14:29 +0000
+++ charmhelpers/payload/archive.py	2013-07-10 18:59:30 +0000
@@ -19,7 +19,7 @@
             return extract_zipfile
     else:
         # look at the file name
-        for ext in ('.tar.gz', '.tgz', 'tar.bz2', '.tbz2', '.tbz'):
+        for ext in ('.tar', '.tar.gz', '.tgz', 'tar.bz2', '.tbz2', '.tbz'):
             if archive_name.endswith(ext):
                 return extract_tarfile
         for ext in ('.zip', '.jar'):
@@ -28,7 +28,8 @@
 
 
 def archive_dest_default(archive_name):
-    return os.path.join(hookenv.charm_dir(), "archives", archive_name)
+    archive_file = os.path.basename(archive_name)
+    return os.path.join(hookenv.charm_dir(), "archives", archive_file)
 
 
 def extract(archive_name, destpath=None):
@@ -38,7 +39,7 @@
             destpath = archive_dest_default(archive_name)
         if not os.path.isdir(destpath):
             host.mkdir(destpath)
-        get_archive_handler(archive_name)(archive_name, destpath)
+        handler(archive_name, destpath)
         return destpath
     else:
         raise ArchiveError("No handler for archive")
 
=== modified file 'tests/fetch/test_archiveurl.py'
--- tests/fetch/test_archiveurl.py	2013-06-26 22:14:29 +0000
+++ tests/fetch/test_archiveurl.py	2013-07-10 18:59:30 +0000
@@ -6,7 +6,11 @@
     patch,
     mock_open,
 )
-from charmhelpers.fetch import archiveurl
+from charmhelpers.fetch import (
+    archiveurl,
+    UnhandledSource,
+)
+import urllib2
 
 
 class ArchiveUrlFetchHandlerTest(TestCase):
@@ -59,8 +63,9 @@
             _open.assert_called_once_with("foo", 'w')
             _open().write.assert_called_with("bar")
 
+    @patch('charmhelpers.fetch.archiveurl.mkdir')
     @patch('charmhelpers.fetch.archiveurl.extract')
-    def test_installs(self, _extract):
+    def test_installs(self, _extract, _mkdir):
         self.fh.download = MagicMock()
 
         for url in self.valid_urls:
@@ -72,3 +77,13 @@
             self.fh.download.assert_called_with(url, dest)
             _extract.assert_called_with(dest)
             self.assertEqual(where, dest)
+
+        url = "http://www.example.com/archive.tar.gz"
+
+        self.fh.download.side_effect = urllib2.URLError('fail')
+        with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
+            self.assertRaises(UnhandledSource, self.fh.install, url)
+
+        self.fh.download.side_effect = OSError('fail')
+        with patch.dict('os.environ', {'CHARM_DIR': 'foo'}):
+            self.assertRaises(UnhandledSource, self.fh.install, url)
 
=== modified file 'tests/payload/test_archive.py'
--- tests/payload/test_archive.py	2013-06-27 15:59:39 +0000
+++ tests/payload/test_archive.py	2013-07-10 18:59:30 +0000
@@ -33,13 +33,15 @@
         zip_archive_handler = archive.extract_zipfile
         _isfile.return_value = False
 
-        for ext in ('tar.gz', 'tgz', 'tar.bz2', 'tbz2', 'tbz'):
+        for ext in ('tar', 'tar.gz', 'tgz', 'tar.bz2', 'tbz2', 'tbz'):
             handler = archive.get_archive_handler("somefile.{}".format(ext))
-            self.assertEqual(handler, tar_archive_handler)
+            msg = "handler for extension: {}".format(ext)
+            self.assertEqual(handler, tar_archive_handler, msg)
 
         for ext in ('zip', 'jar'):
             handler = archive.get_archive_handler("somefile.{}".format(ext))
-            self.assertEqual(handler, zip_archive_handler)
+            msg = "handler for extension {}".format(ext)
+            self.assertEqual(handler, zip_archive_handler, msg)
 
     @patch('zipfile.is_zipfile')
     @patch('tarfile.is_tarfile')
@@ -66,6 +68,9 @@
         thedir = archive.archive_dest_default("baz")
         self.assertEqual(thedir, os.path.join("foo", "archives", "baz"))
 
+        thedir = archive.archive_dest_default("baz/qux")
+        self.assertEqual(thedir, os.path.join("foo", "archives", "qux"))
+
     def test_extracts_tarfile(self):
         destdir = mkdtemp()
         self.addCleanup(rmtree, destdir)