On Thu, May 07, 2009 at 11:42:49AM -0700, Dan Price wrote:
> On Thu 07 May 2009 at 10:38AM, Danek Duvall wrote:
> > On Thu, May 07, 2009 at 02:11:17AM -0700, Dan Price wrote:
> >
> > > > >http://cr.opensolaris.org/~dp/pkg-zonefixes3/
> > >
> > > Webrev is reposted
> >
> > Looks fine to me; just a few nits.
> >
> > attach:
> >
> > - line 425: is GZ_IMAGE something you'd expect to set from outside the
> > script? If not, I'm not sure I understand its purpose.
>
> In attach it is a parallel to GZ_IMAGE in pkgcreatezone. It's
> essentially a hook for a developer (me) to be able to reset what
> the notion of the global zone is. I can take it out, but it's
> harmless, and was amazingly usefull in my testing, where I just
> edited it to be /tmp/somethingelse, an image representing a different
> global zone configuration. I would like to keep it.
I've no problems with keeping it. I just wondered if you wanted something
like
GZ_IMAGE=${GZ_IMAGE:-/}
instead.
> > - line 275: You could create the image with --no-refresh earlier, copy
> > the keys in, and then do a refresh here ...
>
> Ok, I will try that out-- I think if I do:
>
> image-create --no-refresh
> copy keys
> set-publisher
>
> That I should get a refresh on the set-publisher, right?
I think so, but obviously test.
> > client.py:
> >
> > - line 1665: no need to pass in img_dir if you're passing img, right?
>
> I wasn't sure I understood the nuance of "mydir" in the caller, and
> was worried about creating extraneous change. Do you want me to chase it?
If you're uncertain, don't; that's not for last minute stuff. :)
> > - line 1794, 1795: do you need abspath() here? Do you really expect
> > img.root not to be absolute? And probably should use os.path.join()
> > here, too.
>
> os.path.join is the wrong choice here, because of it's subtle semantics.
> If ssl_cert is: /var/pkg/ssl and img_root is: /foo/bar, then
> os.path.join(img_root, ssl_cert) --> "/var/pkg/ssl". You can read all
> about it in the doc for os.path.join.
Yeah. Okay.
> > - line 281: why str()?
>
> Dunno, it was there. I think it's because this could be None, so
> str'ing it gets you to "None".
Which you then string compare to "None", rather than doing object
comparison against None. Seems like extra work to me, and makes it a bit
less clean, but it's no biggie.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
