Merge lp:~yellow/charm-tools/trunk into lp:~charmers/charm-tools/trunk

[Brad Crittenden]

Update parsing of 'juju status' output to account for change in tokens ('state' -> 'agent-state').

[Juan L. Negron (negronjl)]

Reviewing this now.

-Juan

[Juan L. Negron (negronjl)]

Hello:

Thanks for the contribution! Wow this was a mouthful :)

I see a few things that need to be addressed:

- This depends on python-testtools and python shelltoolbox
  I couldn't find shelltoolbox on the archive ( I did find it here: https://launchpad.net/python-shelltoolbox )
  The package dependency should be addressed as this package will fail testing at least.
  The necessary changes should be made to fix the failed dependency on shelltoolbox (branch, include the package, etc.)

Would you address the above and resubmit ?

Thanks,

Juan

review: Resubmit

[Brad Crittenden (bac)]

Hi Juan,

Thank you for the review and I apologize for not responding sooner but I missed the email.

We have a current action item for Clint to provide packaging for python-shelltoolbox and for packaging the python parts of charm-tools. We'll need him to complete those two tasks before this code can be merged.

[Mark Mims (mark-mims)]

On 06/18/2012 09:10 AM, Brad Crittenden wrote:
> Hi Juan,
>
> Thank you for the review and I apologize for not responding sooner but I missed the email.
>
> We have a current action item for Clint to provide packaging for python-shelltoolbox and for packaging the python parts of charm-tools. We'll need him to complete those two tasks before this code can be merged.
Hey Brad, do you have individual bugs for those? Juan and I are trying
to handle what we can for Clint right now (newborn baby and all).

--
Mark Mims, Ph.D.
Canonical Ltd.
<email address hidden>
[phone]

[Brad Crittenden (bac)]

Hi Mark,

We communicated with Clint informally for those tasks, IIRC. I'll open bugs and link them here.

[Brad Crittenden (bac)]

Hi Mark,

I've opened bug 1016585 against lp:python-shelltoolbox and bug 1016588 against charm-tools for the two packaging tasks that we'd previously discussed with Clint. Let me know if you have questions.

[Brad Crittenden (bac)]

Hi Juan,

python-shelltoolbox has been accepted into Sid and Quantal. The dependency would still break on Precise and earlier unless we add python-shelltoolbox to the juju PPA.

What do you recommend?

[Juan L. Negron (negronjl)]

Hi Brad:

I have no objections to adding python-shelltoolbox to the juju PPA. Let's
see what Clint and Mark say about it. It would require more than just my
+1 on this for it to happen.

Mark?

Clint?

Thanks,

Juan

On Thu, Aug 30, 2012 at 7:09 AM, Brad Crittenden <email address hidden> wrote:

> Hi Juan,
>
> python-shelltoolbox has been accepted into Sid and Quantal. The
> dependency would still break on Precise and earlier unless we add
> python-shelltoolbox to the juju PPA.
>
> What do you recommend?
> --
> https://code.launchpad.net/~yellow/charm-tools/trunk/+merge/101554
> You are reviewing the proposed merge of lp:~yellow/charm-tools/trunk into
> lp:charm-tools.
>

[Mark Mims (mark-mims)]

no complaints from me... please go ahead.

Thanks for putting that package together!!

On Thu, Aug 30, 2012 at 09:35:31AM -0700, Juan Negron wrote:
> Hi Brad:
>
> I have no objections to adding python-shelltoolbox to the juju PPA. Let's
> see what Clint and Mark say about it. It would require more than just my
> +1 on this for it to happen.
>
> Mark?
>
> Clint?
>
> Thanks,
>
> Juan
>
>
>
> On Thu, Aug 30, 2012 at 7:09 AM, Brad Crittenden <email address hidden> wrote:
>
> > Hi Juan,
> >
> > python-shelltoolbox has been accepted into Sid and Quantal. The
> > dependency would still break on Precise and earlier unless we add
> > python-shelltoolbox to the juju PPA.
> >
> > What do you recommend?
> > --
> > https://code.launchpad.net/~yellow/charm-tools/trunk/+merge/101554
> > You are reviewing the proposed merge of lp:~yellow/charm-tools/trunk into
> > lp:charm-tools.
> >

--
Mark Mims, Ph.D.
Ubuntu Server Team
Canonical Ltd.
<email address hidden>
[phone]

[Brad Crittenden (bac)]

Hi Juan and Mark,

Yesterday Clint merged Graham's branch that included the python module into charm tools. This branch adds some necessary fixes to be current with juju changes that occurred after Graham submitted his branch. Without the changes in this branch the python helpers are broken.

The review should be much easier than the previous version and getting it merged straightforward.

Thanks,
Brad

[Juan L. Negron (negronjl)]

This LGTM. Approved.

-Juan

review: Approve

[Brad Crittenden (bac)]

Juan could you mark this proposal as 'Approved' and please land it for me as I don't have permission.

=== modified file 'helpers/python/charmhelpers/__init__.py'
--- helpers/python/charmhelpers/__init__.py	2012-03-16 15:11:45 +0000
+++ helpers/python/charmhelpers/__init__.py	2012-08-31 13:20:31 +0000
@@ -12,6 +12,10 @@
     'relation_get',
     'relation_set',
     'unit_info',
+    'wait_for_machine',
+    'wait_for_page_contents',
+    'wait_for_relation',
+    'wait_for_unit',
     ]
 
 from collections import namedtuple
@@ -19,7 +23,6 @@
 import operator
 from shelltoolbox import (
     command,
-    run,
     script_name,
     )
 import tempfile
@@ -28,6 +31,7 @@
 import yaml
 
 
+SLEEP_AMOUNT = 0.1
 Env = namedtuple('Env', 'uid gid home')
 log = command('juju-log')
 # We create a juju_status Command here because it makes testing much,
@@ -125,14 +129,14 @@
         if len(non_zookeeper_machines) >= num_machines:
             all_machines_running = True
             for machine in non_zookeeper_machines:
-                if machine['instance-state'] != 'running':
+                if machine.get('instance-state') != 'running':
                     all_machines_running = False
                     break
             if all_machines_running:
                 break
         if time.time() - start_time >= timeout:
             raise RuntimeError('timeout waiting for service to start')
-        time.sleep(0.1)
+        time.sleep(SLEEP_AMOUNT)
     return num_machines, time.time() - start_time
 
 
@@ -141,14 +145,14 @@
     wait_for_machine(num_machines=1)
     start_time = time.time()
     while True:
-        state = unit_info(service_name, 'state')
+        state = unit_info(service_name, 'agent-state')
         if 'error' in state or state == 'started':
             break
         if time.time() - start_time >= timeout:
             raise RuntimeError('timeout waiting for service to start')
-        time.sleep(0.1)
+        time.sleep(SLEEP_AMOUNT)
     if state != 'started':
-        raise RuntimeError('unit did not start, state: ' + state)
+        raise RuntimeError('unit did not start, agent-state: ' + state)
 
 
 def wait_for_relation(service_name, relation_name, timeout=120):
@@ -160,7 +164,7 @@
             break
         if time.time() - start_time >= timeout:
             raise RuntimeError('timeout waiting for relation to be up')
-        time.sleep(0.1)
+        time.sleep(SLEEP_AMOUNT)
 
 
 def wait_for_page_contents(url, contents, timeout=120, validate=None):
@@ -178,4 +182,4 @@
                 return page
         if time.time() - start_time >= timeout:
             raise RuntimeError('timeout waiting for contents of ' + url)
-        time.sleep(0.1)
+        time.sleep(SLEEP_AMOUNT)

=== modified file 'helpers/python/charmhelpers/tests/test_charmhelpers.py'
--- helpers/python/charmhelpers/tests/test_charmhelpers.py	2012-03-19 11:50:05 +0000
+++ helpers/python/charmhelpers/tests/test_charmhelpers.py	2012-08-31 13:20:31 +0000
@@ -1,6 +1,5 @@
 # Tests for Python charm helpers.
 
-import charmhelpers
 import unittest
 import yaml
 
@@ -8,6 +7,13 @@
 from StringIO import StringIO
 from testtools import TestCase
 
+import sys
+# Path hack to ensure we test the local code, not a version installed in
+# /usr/local/lib.  This is necessary since /usr/local/lib is prepended before
+# what is specified in PYTHONPATH.
+sys.path.insert(0, 'helpers/python')
+import charmhelpers
+
 
 class CharmHelpersTestCase(TestCase):
     """A basic test case for Python charm helpers."""
@@ -56,7 +62,7 @@
                 'relations': {
                     'db': {'state': 'up'},
                     },
-                'state': unit_state,
+                'agent-state': unit_state,
                 }
             service_data['units']['{}/{}'.format(service_name, i)] = (
                 unit_data)
@@ -131,9 +137,9 @@
         self.patch(charmhelpers, 'juju_status', mock_juju_status)
         self.assertEqual(
             'pending',
-            charmhelpers.unit_info('test-service', 'state'))
+            charmhelpers.unit_info('test-service', 'agent-state'))
 
-    def test_unit_info_returns_empty_for_nonexistant_service(self):
+    def test_unit_info_returns_empty_for_nonexistent_service(self):
         # If the service passed to unit_info() has not yet started (or
         # otherwise doesn't exist), unit_info() will return an empty
         # string.

=== modified file 'setup.py'
--- setup.py	2012-03-14 22:13:47 +0000
+++ setup.py	2012-08-31 13:20:31 +0000
@@ -10,7 +10,7 @@
 
 from setuptools import setup, find_packages
 
-__version__ = '0.0.1'
+__version__ = '0.0.3'
 
 
 setup(