On 06/01/12 14:06, Darren Kenny wrote:
On 01/06/2012 13:57, Drew Fisher wrote:
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
"""
OK, but really sounds like Fileysystem.create() should default dry_run to
False TB looks strange like this - making this change wouldn't effect any
existing uses since they had to specify a value in the past.
Makes sense, I've changed Filesystem.create() and remove the explicit
setting in these 4 calls.
New webrev posted :
https://cr.opensolaris.org/action/browse/caiman/mattman/7057261-7171068-7173255/
cheers
Matt
- 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.
I know the API, but on first reading it's not really clear that it does a
direct request to the existing dataset.
- 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/
Looks better to me now :)
I'm about to take off for the weekend, so I'm going to transfer any
further issues to Matt to handle.
OK, have a good weekend.
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
