Merge lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list into lp:charm-helpers
Proposed by
Chris MacNaughton
Status: | Merged |
---|---|
Merged at revision: | 562 |
Proposed branch: | lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list |
Merge into: | lp:charm-helpers |
Diff against target: |
121 lines (+76/-10) 2 files modified
charmhelpers/contrib/amulet/utils.py (+15/-10) tests/contrib/amulet/test_utils.py (+61/-0) |
To merge this branch: | bzr merge lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ryan Beisner (community) | Approve | ||
charmers | Pending | ||
Review via email: mp+288014@code.launchpad.net |
Description of the change
Allow the validate_
To post a comment you must log in.
1. See inline comments. That existing validation block is already particularly non-beautiful ;-) and the commentary helps clarify. Let's keep that consistent in that block.
2. The variable name e_pids_length (expected process IDs length) makes sense for the existing validation blocks, but it is confusing/mis-named in the context of it being a list type. Ideas to address that?
3. I'd like to see this validated before merge by syncing this c-h LP branch into ceph-mon, then updating basic_deployment.py like so:
<icey> a ceph-mon test /review. openstack. org/#/c/ 287446/ 7/tests/ basic_deploymen t.py
<icey> looking...
<icey> beisner: https:/
<icey> line 185, changed from {'ceph-osd': 2} to {'ceph-osd': True}
<icey> would rather be: {'ceph-osd': [2, 3]}
4. And finally by running the test, capturing and commenting here with a pastebin of the test
output.