Looks good to me now... Thanks,
Darren. ----- [email protected] wrote: > 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
