Merge lp:~hazmat/pyjuju/repository-broken-charms into lp:pyjuju

[Kapil Thangavelu]

Charm local repositories should always process all their charms for a search.

Changes

  - add catch all handler to repository for unexpected errors (os)
  - rearrange charm exception hierarchy
    - charmnotfound is not a charm structural error
    - metadata errors are charm errors.

https://codereview.appspot.com/5841056/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+98062_code.launchpad.net,

Message:
Please take a look.

Description:
Charm local repositories should always process all their charms for a
search.

Changes

   - add catch all handler to repository for unexpected errors (os)
   - rearrange charm exception hierarchy
     - charmnotfound is not a charm structural error
     - metadata errors are charm errors.

https://code.launchpad.net/~hazmat/juju/repository-broken-charms/+merge/98062

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/5841056/

Affected files:
   A [revision details]
   M juju/charm/errors.py
   M juju/charm/repository.py
   M juju/charm/tests/test_errors.py
   M juju/charm/tests/test_repository.py

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/5841056/diff/1/juju/charm/errors.py
File juju/charm/errors.py (right):

https://codereview.appspot.com/5841056/diff/1/juju/charm/errors.py#newcode30
juju/charm/errors.py:30: """Raised when an error in the info file of a
charm is found."""
MetaDataError seems to be missing a test; might be worth adding one,
however trivial it may be; and it also appears to have no methods that
need to exist.

https://codereview.appspot.com/5841056/

[Kapil Thangavelu (hazmat)]

added md error test, the methods are nesc. else we get charmerror behavior which expects two args, instead of juju error behavior (error string). if md is broken we can't satisify the name arg of the charmerror, since we don't definitively know the name.

=== modified file 'juju/charm/errors.py'
--- juju/charm/errors.py	2012-02-11 03:34:23 +0000
+++ juju/charm/errors.py	2012-03-17 16:28:20 +0000
@@ -1,6 +1,21 @@
 from juju.errors import CharmError, JujuError
 
 
+class CharmNotFound(JujuError):
+    """A charm was not found in the repository."""
+
+    # This isn't semantically an error with a charm error, its an error
+    # even finding the charm.
+
+    def __init__(self, repository_path, charm_name):
+        self.repository_path = repository_path
+        self.charm_name = charm_name
+
+    def __str__(self):
+        return "Charm '%s' not found in repository %s" % (
+            self.charm_name, self.repository_path)
+
+
 class CharmURLError(CharmError):
 
     def __init__(self, url, message):
@@ -11,21 +26,15 @@
         return "Bad charm URL %r: %s" % (self.url, self.message)
 
 
-class CharmNotFound(CharmError):
-    """A charm was not found in the repository."""
-
-    def __init__(self, repository_path, charm_name):
-        self.repository_path = repository_path
-        self.charm_name = charm_name
-
-    def __str__(self):
-        return "Charm '%s' not found in repository %s" % (
-            self.charm_name, self.repository_path)
-
-
-class MetaDataError(JujuError):
+class MetaDataError(CharmError):
     """Raised when an error in the info file of a charm is found."""
 
+    def __init__(self, *args):
+        super(CharmError, self).__init__(*args)
+
+    def __str__(self):
+        return super(CharmError, self).__str__()
+
 
 class InvalidCharmHook(CharmError):
     """A named hook was not found to be valid for the charm."""

=== modified file 'juju/charm/repository.py'
--- juju/charm/repository.py	2012-02-16 19:19:42 +0000
+++ juju/charm/repository.py	2012-03-17 16:28:20 +0000
@@ -9,13 +9,13 @@
 from twisted.web.client import downloadPage, getPage
 from twisted.web.error import Error
 
-from juju.charm.errors import MetaDataError, ServiceConfigError
 from juju.charm.provider import get_charm_from_path
 from juju.charm.url import CharmURL
 from juju.errors import FileNotFound
 from juju.lib import under
 
-from .errors import CharmNotFound, CharmError, RepositoryNotFound
+from .errors import (
+    CharmNotFound, CharmError, RepositoryNotFound, ServiceConfigValueError)
 
 log = logging.getLogger("juju.charm")
 
@@ -59,13 +59,18 @@
                 # Log yaml errors for feedback to developers.
                 log.warning("Charm %r has a YAML error: %s", dentry, e)
                 continue
-            except (ServiceConfigError, MetaDataError), e:
+            except (CharmError, ServiceConfigValueError), e:
                 # Log invalid config.yaml and metadata.yaml semantic errors
                 log.warning("Charm %r has an error: %r %s", dentry, e, e)
                 continue
-            except CharmError:
+            except CharmNotFound:
                 # This could just be a random directory/file in the repo
                 continue
+            except Exception, e:
+                # Catch all (perms, unknowns, etc)
+                log.warning(
+                    "Unexpected error while processing %s: %r",
+                    dentry, e)
 
     def find(self, charm_url):
         """Find a charm with the given name.

=== modified file 'juju/charm/tests/test_errors.py'
--- juju/charm/tests/test_errors.py	2012-02-11 03:34:23 +0000
+++ juju/charm/tests/test_errors.py	2012-03-17 16:28:20 +0000
@@ -29,7 +29,7 @@
         self.assertEquals(
             str(error),
             "Charm 'cassandra' not found in repository /path")
-        self.assertTrue(isinstance(error, CharmError))
+        self.assertTrue(isinstance(error, JujuError))
 
     def test_InvalidCharmHook(self):
         error = InvalidCharmHook("mysql", "magic-relation-changed")

=== modified file 'juju/charm/tests/test_repository.py'
--- juju/charm/tests/test_repository.py	2012-03-08 02:50:43 +0000
+++ juju/charm/tests/test_repository.py	2012-03-17 16:28:20 +0000
@@ -2,7 +2,6 @@
 import os
 import inspect
 import shutil
-import tempfile
 
 from twisted.internet.defer import fail, inlineCallbacks, succeed
 from twisted.web.error import Error
@@ -12,6 +11,7 @@
 from juju.charm.repository import (
     LocalCharmRepository, RemoteCharmRepository, resolve)
 from juju.charm.url import CharmURL
+from juju.charm import provider
 from juju.errors import CharmError
 from juju.lib import under
 
@@ -57,6 +57,25 @@
         # define repository objects
         self.repository1 = LocalCharmRepository(self.unbundled_repo_path)
         self.repository2 = LocalCharmRepository(self.bundled_repo_path)
+        self.output = self.capture_logging("juju.charm")
+
+    def assert_there(self, name, repo, revision, latest_revision=None):
+        url = self.charm_url(name)
+        charm = yield repo.find(url)
+        self.assertEquals(charm.get_revision(), revision)
+        latest = yield repo.latest(url)
+        self.assertEquals(latest, latest_revision or revision)
+
+    @inlineCallbacks
+    def assert_not_there(self, name, repo, revision=None):
+        url = self.charm_url(name)
+        msg = "Charm 'local:series/%s' not found in repository %s" % (
+            name, repo.path)
+        err = yield self.assertFailure(repo.find(url), CharmNotFound)
+        self.assertEquals(str(err), msg)
+        if revision is None:
+            err = yield self.assertFailure(repo.latest(url), CharmNotFound)
+            self.assertEquals(str(err), msg)
 
     def charm_url(self, name):
         return CharmURL.parse("local:series/" + name)
@@ -74,17 +93,6 @@
         err = self.assertRaises(RepositoryNotFound, LocalCharmRepository, path)
         self.assertEquals(str(err), "No repository found at %r" % path)
 
-    @inlineCallbacks
-    def assert_not_there(self, name, repo, revision=None):
-        url = self.charm_url(name)
-        msg = "Charm 'local:series/%s' not found in repository %s" % (
-            name, repo.path)
-        err = yield self.assertFailure(repo.find(url), CharmNotFound)
-        self.assertEquals(str(err), msg)
-        if revision is None:
-            err = yield self.assertFailure(repo.latest(url), CharmNotFound)
-            self.assertEquals(str(err), msg)
-
     def test_find_inappropriate_url(self):
         url = CharmURL.parse("cs:foo/bar")
         err = self.assertRaises(AssertionError, self.repository1.find, url)
@@ -93,12 +101,31 @@
     def test_completely_missing(self):
         return self.assert_not_there("zebra", self.repository1)
 
-    def test_unkown_files_ignored(self):
+    def test_unknown_files_ignored(self):
         self.makeFile(
             "Foobar",
             path=os.path.join(self.repository1.path, "series", "zebra"))
         return self.assert_not_there("zebra", self.repository1)
 
+    @inlineCallbacks
+    def test_random_error_logged(self):
+        get_charm = self.mocker.replace(provider.get_charm_from_path)
+        get_charm(ANY)
+        self.mocker.throw(SyntaxError("magic"))
+        self.mocker.count(0, 3)
+        self.mocker.replay()
+
+        yield self.assertFailure(
+            self.repository1.find(self.charm_url("zebra")),
+            CharmNotFound)
+
+        self.assertIn(
+            "Unexpected error while processing",
+            self.output.getvalue())
+        self.assertIn(
+            "SyntaxError('magic',)",
+            self.output.getvalue())
+
     def test_unknown_directories_ignored(self):
         self.makeDir(
             path=os.path.join(self.repository1.path, "series", "zebra"))
@@ -116,10 +143,9 @@
 revision: 0
 summary: hola""")
         fh.close()
-        stream = self.capture_logging("juju.charm")
         yield self.assertFailure(
             self.repository1.find(self.charm_url("zebra")), CharmNotFound)
-        output = stream.getvalue()
+        output = self.output.getvalue()
         self.assertIn(
             "Charm 'zebra' has a YAML error", output)
         self.assertIn(
@@ -140,9 +166,8 @@
 revision: 0
 summary: hola""")
         fh.close()
-        stream = self.capture_logging("juju.charm")
         yield self.repository1.find(self.charm_url("sample"))
-        output = stream.getvalue()
+        output = self.output.getvalue()
         self.assertIn(
             "Charm 'mysql' has a YAML error", output)
         self.assertIn(
@@ -169,13 +194,6 @@
     def test_repo_type(self):
         self.assertEqual(self.repository1.type, "local")
 
-    def assert_there(self, name, repo, revision, latest_revision=None):
-        url = self.charm_url(name)
-        charm = yield repo.find(url)
-        self.assertEquals(charm.get_revision(), revision)
-        latest = yield repo.latest(url)
-        self.assertEquals(latest, latest_revision or revision)
-
     @inlineCallbacks
     def test_success_unbundled(self):
         yield self.assert_there("sample", self.repository1, 2)