Merge lp:~benji/charmworld/1263120-allow-self-referential-relations-in-bundles into lp:charmworld

[Benji York]

Add a very useful hint about relation ordering.

Add a very useful hint about relation ordering.

https://codereview.appspot.com/54560044/

[Benji York (benji)]

Reviewers: mp+202373_code.launchpad.net,

Message:
Please take a look.

Description:
Add a very useful hint about relation ordering.

https://code.launchpad.net/~benji/charmworld/1263120-allow-self-referential-relations-in-bundles/+merge/202373

(do not edit description out of merge proposal)

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

Affected files (+44, -7 lines):
   A [revision details]
   M charmworld/lib/proof.py
   M charmworld/lib/tests/test_proof.py
   M charmworld/views/tests/test_proof.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140117153416-xbkyontzme0m6h82
+New revision: <email address hidden>

Index: charmworld/lib/proof.py
=== modified file 'charmworld/lib/proof.py'
--- charmworld/lib/proof.py 2014-01-13 18:31:15 +0000
+++ charmworld/lib/proof.py 2014-01-20 20:33:00 +0000
@@ -91,7 +91,8 @@
      if not exist or not same_type:
          # Then this definition is invalid.
          msg = ('The requested relation %s to %s is incompatible between '
- 'services.')
+ 'services. Hint: if order of the relation were reversed
it '
+ 'would be valid.')
          raise ProofError(
              [('Endpoints are the same type on both services? ' +
                str(same_type)), ],

Index: charmworld/lib/tests/test_proof.py
=== modified file 'charmworld/lib/tests/test_proof.py'
--- charmworld/lib/tests/test_proof.py 2014-01-13 18:31:15 +0000
+++ charmworld/lib/tests/test_proof.py 2014-01-20 20:45:50 +0000
@@ -308,10 +308,9 @@
                  requires,
                  provides)

- msg = ('The requested relation db to cache is incompatible
between '
- 'services.')
- self.assertEqual(
- msg,
+ self.assertIn(
+ 'The requested relation db to cache is incompatible between '
+ 'services.',
              exc.exception.msg
          )
          self.assertEqual(

Index: charmworld/views/tests/test_proof.py
=== modified file 'charmworld/views/tests/test_proof.py'
--- charmworld/views/tests/test_proof.py 2014-01-14 19:24:49 +0000
+++ charmworld/views/tests/test_proof.py 2014-01-20 20:33:00 +0000
@@ -12,8 +12,8 @@
      """Verify that a bundle can be proofed in charmworld."""

      def setup_relatable_charms(self, second_requires, second_provides):
- # The default charm provides website:https and requires
- # database:mongodb
+ # Unless otherwise specified, the generated charm provides
+ # website:https and requires database:mongodb.
          _id, charm_one = factory.makeCharm(
              self.db,
              description='',
@@ -278,6 +278,41 @@
              'wiki: The requested relation db to proxy is incompatible',
              response.json_body['error_messages'][0])

+ def test_reverse_relation_order(self):
+ # Relations must be provided in (provides, requires) order. If a
+ # reverse relation would have worked, a nice error message tells
you
+ # so.
+ ...

[Richard Harding (rharding)]

LGTM with the one question on the assumption in the hint.

https://codereview.appspot.com/54560044/diff/1/charmworld/lib/proof.py
File charmworld/lib/proof.py (right):

https://codereview.appspot.com/54560044/diff/1/charmworld/lib/proof.py#newcode96
charmworld/lib/proof.py:96: raise ProofError(
Doesn't this hint need to account for the case of non matching types?

https://codereview.appspot.com/54560044/

[Benji York (benji)]

Please take a look.

https://codereview.appspot.com/54560044/

[Benji York (benji)]

Fixed bug found in review.

https://codereview.appspot.com/54560044/diff/1/charmworld/lib/proof.py
File charmworld/lib/proof.py (right):

https://codereview.appspot.com/54560044/diff/1/charmworld/lib/proof.py#newcode96
charmworld/lib/proof.py:96: raise ProofError(
On 2014/01/20 20:53:31, rick.harding wrote:
> Doesn't this hint need to account for the case of non matching types?

Indeed! I've fixed this and added a test.

https://codereview.appspot.com/54560044/

[Juju Gui Bot (juju-gui-bot)]

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~benji/charmworld/1263120-allow-self-referential-relations-in-bundles/+merge/202373/+edit-commit-message

review: Needs Fixing (continuous-integration)

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

[Kapil Thangavelu (hazmat)]

order within a relation doesn't matter.

review: Disapprove

[Benji York (benji)]

Changing proof's behavior would be fine, but this branch doesn't introduce the behavior in question (requiring a particular order for relations) it only reports it to the user better.

=== modified file 'charmworld/lib/proof.py'
--- charmworld/lib/proof.py	2014-01-13 18:31:15 +0000
+++ charmworld/lib/proof.py	2014-01-21 13:47:04 +0000
@@ -92,6 +92,9 @@
         # Then this definition is invalid.
         msg = ('The requested relation %s to %s is incompatible between '
                'services.')
+        if reversed:
+            msg += ('Hint: if order of the relation were reversed it '
+                    'would be valid.')
         raise ProofError(
             [('Endpoints are the same type on both services? ' +
               str(same_type)), ],

=== modified file 'charmworld/lib/tests/test_proof.py'
--- charmworld/lib/tests/test_proof.py	2014-01-13 18:31:15 +0000
+++ charmworld/lib/tests/test_proof.py	2014-01-21 13:47:04 +0000
@@ -308,10 +308,9 @@
                 requires,
                 provides)
 
-        msg = ('The requested relation db to cache is incompatible between '
-               'services.')
-        self.assertEqual(
-            msg,
+        self.assertIn(
+            'The requested relation db to cache is incompatible between '
+            'services.',
             exc.exception.msg
         )
         self.assertEqual(
@@ -319,6 +318,33 @@
             exc.exception.debug_info[0]
         )
 
+    def test_including_hint_when_relation_direction_is_reversed(self):
+        # If the relation is not valid, but would have been if the direction
+        # of the relation had been reversed, then a helpful hint is included
+        # in the exception.
+
+        # If these two were reversed, the relation would have been valid.
+        check_requires = 'cache'
+        check_provides = 'website'
+
+        requires = {'website': {'interface': 'http'}}
+        provides = {'cache': {'interface': 'http'}}
+
+        with self.assertRaises(ProofError) as exc:
+            verify_points_exist_and_same_type(
+                check_requires, check_provides, requires, provides)
+
+        # Since the relations are reversed, a hint is given.
+        self.assertIn(
+            'Hint: if order of the relation were reversed it would be valid.',
+            exc.exception.msg
+        )
+        # The normal incompatability message is included too.
+        self.assertIn(
+            'The requested relation cache to website is incompatible',
+            exc.exception.msg
+        )
+
     def test_valid_fully_qualified_endpoints(self):
         # relation = ['wiki:website', 'haproxy:cache']
         check_requires = 'website'

=== modified file 'charmworld/views/tests/test_proof.py'
--- charmworld/views/tests/test_proof.py	2014-01-14 19:24:49 +0000
+++ charmworld/views/tests/test_proof.py	2014-01-21 13:47:04 +0000
@@ -12,8 +12,8 @@
     """Verify that a bundle can be proofed in charmworld."""
 
     def setup_relatable_charms(self, second_requires, second_provides):
-        # The default charm provides website:https and requires
-        # database:mongodb
+        # Unless otherwise specified, the generated charm provides
+        # website:https and requires database:mongodb.
         _id, charm_one = factory.makeCharm(
             self.db,
             description='',
@@ -278,6 +278,41 @@
             'wiki: The requested relation db to proxy is incompatible',
             response.json_body['error_messages'][0])
 
+    def test_reverse_relation_order(self):
+        # Relations must be provided in (provides, requires) order.  If a
+        # reverse relation would have worked, a nice error message tells you
+        # so.
+        one_store_url, two_store_url = self.setup_relatable_charms(
+            {}, {'db': {'interface': 'mongodb'}})
+
+        bundle_base = dedent("""\
+            wiki:
+                series: precise
+                promulgated: True
+                services:
+                    wiki:
+                        charm: {}
+                    db:
+                        charm: {}
+                relations:
+                    - ["db:db", "wiki:database"]
+            """.format(one_store_url, two_store_url))
+
+        request = self.make_request('bundle', remainder='/proof')
+        request.params = {
+            'deployer_file': bundle_base
+        }
+        response = self.api_class(request)()
+        self.assertEqual(200, response.status_code)
+        error_messages = response.json_body['error_messages']
+        self.assertEqual(1, len(error_messages))
+        self.assertIn(
+            'wiki: The requested relation db to database is incompatible',
+            error_messages[0])
+        self.assertIn(
+            'Hint: if order of the relation were reversed it would be valid.',
+            error_messages[0])
+
     def test_bundle_proof_passes_with_valid_relations(self):
         one_store_url, two_store_url = self.setup_relatable_charms(
             {