Darren,
Thanks for the review. Comments in-line...
On 6/1/12 4:11 AM, Darren Kenny wrote:
Hi Drew,
I took a look over the changes you made, and have some comments:
In distro_const/__init__.py:
- Lines 272, 283:
Should we also check that there is actually 1 Filesystem and 1 Zpool,
since the subsequent code assumes fs[0] and zpool[0] vars are valid.
We probably should.
- Lines 318 to 321:
Why do you have to specifically set dry_run here? Should you not be also
passing this value on from self.dry_run or similar since not doing so
makes the dry_run parameters to DC next to useless since it's then
destructive, doesn't it?
There is no self variable at this point in DC. We're not inside any of
the checkpoints and these are all standalone functions. Also,
Filesystem.create() requires the dry_run flag:
def create(self, dry_run):
""" create the filesystem
"""
- Lines 333 ot 339:
Might be useful here to have a specific comment to state the the
fs.get() is fetching the mountpoint directly from ZFS at this
point.
I figured the call to get() was satisfactory, but I'll add a comment.
- Lines 308 and 472:
Would it be worth providing a single point for constructing these
paths rather than repeating the code?
Sure. I'll change 308 to simply reference eng.dataset
In test_distro_const.py:
- Why did you remove the test here? Is it still not useful to test the
validation?
Originally I removed the test because validate_target changed. It used
to process the DOC object and return strings (name, dataset, action,
mountpoint). Now it sanity tests the DOC objects and returns DOC
objects. It seemed rather silly to check them against themselves.
What I've done now is add two new tests:
- test_no_pool
- test_no_filesystem
to make sure the additions added in validate_target for no pool/fs are
properly checked.
I've uploaded a new webrev here:
https://cr.opensolaris.org/action/browse/caiman/drewfish/filesystem_from_xml2/webrev/
I'm about to take off for the weekend, so I'm going to transfer any
further issues to Matt to handle.
Thanks!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
