Merge lp:~jimbaker/pyjuju/juju-status-expose-hint into lp:pyjuju

[Jim Baker]

juju status should hint that a service may need to be exposed

Adjust juju status so that exposed: false is output for a service if there are open ports but not exposed

https://codereview.appspot.com/6206086/

[Jim Baker (jimbaker)]

Reviewers: mp+106474_code.launchpad.net,

Message:
Please take a look.

Description:
juju status should hint that a service may need to be exposed

Adjust juju status so that exposed: false is output for a service if
there are open ports but not exposed

https://code.launchpad.net/~jimbaker/juju/juju-status-expose-hint/+merge/106474

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/control/status.py
   M juju/control/tests/test_status.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:
<email address hidden>
+New revision: <email address hidden>

Index: juju/control/status.py
=== modified file 'juju/control/status.py'
--- juju/control/status.py 2012-04-15 23:03:05 +0000
+++ juju/control/status.py 2012-05-18 21:41:15 +0000
@@ -311,10 +311,14 @@
                                 if unit_connected else "down"

          exposed = self.service_data[service.service_name].get("exposed")
+ open_ports = yield unit.get_open_ports()
          if exposed:
- open_ports = yield unit.get_open_ports()
              u["open-ports"] = ["{port}/{proto}".format(**port_info)
                                 for port_info in open_ports]
+ elif open_ports:
+ # Ensure a hint is provided that there are open ports if
+ # not exposed by setting the key in the output
+ self.service_data[service.service_name]["exposed"] = False

          u["public-address"] = yield unit.get_public_address()

Index: juju/control/tests/test_status.py
=== modified file 'juju/control/tests/test_status.py'
--- juju/control/tests/test_status.py 2012-04-09 19:40:57 +0000
+++ juju/control/tests/test_status.py 2012-05-18 21:41:15 +0000
@@ -425,6 +425,7 @@
          self.assertEqual(
              services["wordpress"],
              {"charm": "local:series/wordpress-3",
+ "exposed": False,
               "relations": {
                      "cache": ["memcache"],
                      "db": ["mysql"],
@@ -624,7 +625,7 @@
          self.assertEqual(set(services["varnish"].keys()),
                           set(["exposed", "charm", "relations", "units"]))
          self.assertEqual(set(services["wordpress"].keys()),
- set(["charm", "relations", "units"]))
+ set(["charm", "exposed", "relations", "units"]))

          for service in services.itervalues():
              self.assertGreaterEqual( # may also include "exposed" key
@@ -671,7 +672,7 @@
          self.assertEqual(set(services["varnish"].keys()),
                           set(["exposed", "charm", "relations", "units"]))
          self.assertEqual(set(services["wordpress"].keys()),
- set(["charm", "relations", "units"]))
+ set(["charm", "exposed", "relations", "units"]))

          for service in services.itervalues():
              self.assertTrue...

[Clint Byrum (clint-fewbar)]

This seems like a change in behavior, even though its a positive one, I think it should be delayed until honolulu.

[Jim Baker (jimbaker)]

Delaying to honolulu is reasonable.

[Kapil Thangavelu (hazmat)]

lgtm

review: Approve

=== modified file 'juju/control/status.py'
--- juju/control/status.py	2012-04-15 23:03:05 +0000
+++ juju/control/status.py	2012-05-18 21:46:18 +0000
@@ -311,10 +311,14 @@
                                if unit_connected else "down"
 
         exposed = self.service_data[service.service_name].get("exposed")
+        open_ports = yield unit.get_open_ports()
         if exposed:
-            open_ports = yield unit.get_open_ports()
             u["open-ports"] = ["{port}/{proto}".format(**port_info)
                                for port_info in open_ports]
+        elif open_ports:
+            # Ensure a hint is provided that there are open ports if
+            # not exposed by setting the key in the output
+            self.service_data[service.service_name]["exposed"] = False
 
         u["public-address"] = yield unit.get_public_address()
 

=== modified file 'juju/control/tests/test_status.py'
--- juju/control/tests/test_status.py	2012-04-09 19:40:57 +0000
+++ juju/control/tests/test_status.py	2012-05-18 21:46:18 +0000
@@ -425,6 +425,7 @@
         self.assertEqual(
             services["wordpress"],
             {"charm": "local:series/wordpress-3",
+             "exposed": False,
              "relations": {
                     "cache": ["memcache"],
                     "db": ["mysql"],
@@ -624,7 +625,7 @@
         self.assertEqual(set(services["varnish"].keys()),
                          set(["exposed", "charm", "relations", "units"]))
         self.assertEqual(set(services["wordpress"].keys()),
-                         set(["charm", "relations", "units"]))
+                         set(["charm", "exposed", "relations", "units"]))
 
         for service in services.itervalues():
             self.assertGreaterEqual(  # may also include "exposed" key
@@ -671,7 +672,7 @@
         self.assertEqual(set(services["varnish"].keys()),
                          set(["exposed", "charm", "relations", "units"]))
         self.assertEqual(set(services["wordpress"].keys()),
-                         set(["charm", "relations", "units"]))
+                         set(["charm", "exposed", "relations", "units"]))
 
         for service in services.itervalues():
             self.assertTrue(service["charm"].startswith("local:series/"))