Code review comment for lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list

Revision history for this message
Ryan Beisner (1chb1n) wrote :

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
<icey> looking...
<icey> beisner: https://review.openstack.org/#/c/287446/7/tests/basic_deployment.py
<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.

review: Needs Fixing

« Back to merge proposal