Merge lp://qastaging/~gnuoy/charm-helpers/cepg-broker into lp://qastaging/charm-helpers
Proposed by
Liam Young
| Status: | Merged |
|---|---|
| Merged at revision: | 447 |
| Proposed branch: | lp://qastaging/~gnuoy/charm-helpers/cepg-broker |
| Merge into: | lp://qastaging/charm-helpers |
| Diff against target: |
812 lines (+612/-30) 5 files modified
charmhelpers/contrib/openstack/context.py (+8/-9) charmhelpers/contrib/storage/linux/ceph.py (+224/-2) tests/contrib/openstack/test_os_contexts.py (+36/-4) tests/contrib/storage/test_linux_ceph.py (+337/-8) tests/helpers.py (+7/-7) |
| To merge this branch: | bzr merge lp://qastaging/~gnuoy/charm-helpers/cepg-broker |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Edward Hope-Morley | Approve | ||
| Chris Glass (community) | Approve | ||
|
Review via email:
|
|||
To post a comment you must log in.
Revision history for this message
| Edward Hope-Morley (hopem) wrote | # |
Revision history for this message
| Edward Hope-Morley (hopem) wrote | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Hi, thanks for your proposal.
The code looks fine in general, but it would really help future readers if you could expand the docstrings a little bit to explain what the problem they are trying to solve is. I understand it now, after some effort, while the bug and context is still fresh, but I (and others) certainly won't in a few months if coming back to it is needed.
An extra bit at the beginning of the file or outside of a particular function about how the newly introduced functions are supposed to be called by whom and in what order would also help tremendously.
I know it seems verbose right now, but it will really save a lot of time if in the future somebody has to look at this code again.
Suggestions inline.