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. >> >> - 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
