Hi Ed,
Thanks for the feedback. I've replied to your design comments and will
incorporate the rest of your feedback into my changes today.
Thanks,
Gary
On Wed, Jan 27, 2010 at 10:10:04PM -0800, Edward Pilatowicz wrote:
> a couple comments below are very nit-picky, so they are prefixed with
> "ffti" and you should "feel free to ignore" them. :)
>
> ----------
> general:
>
> - since the -p output doesn't match the normal output, -p should not
> imply -H. (otherwise how are people supposed to know what the columns
> are without reading the source?)
>
> ---------
> src/brand/common.ksh
>
> - comment nit: are you missing something before "and" here?
> publisher name which can be and URL type which
>
> - nit: our normal shell style for "if" statements has the "then" part on
> the same line as the "if". ie:
> if [ ... ]; then
>
> - get_publisher_attr_args(): i'm really surprised that creating a
> sub-block with "{ ... }" works. i've never seen that syntax before
> (outside function declerations and variable references) and it doesn't
> work in other shells like sh, ksh88, and bash. if you needed a
> sub-block you should use "( ... )", but in this case there is no need
> for a sub-block.
>
> - ffti: personally, i think that using "'"$var"'" with nawk scripts and
> shell veriables is kinda clunky. did you know that nawk supports
> parameters? so instead of doing:
> nawk '$0 == "'"$var"'" {...}'
> you could do:
> nawk '$0 == var {...}' var="$var"
>
> ---------
> src/brand/pkgcreatezone
>
> - could you move the following comment:
> # --no-refresh is used here so that update operations can be
> # coalesced.
> closer to the following code:
> $PKG set-publisher --no-refresh
> right now they seem a little spread apart.
>
> - why bother with SAVE_PKG_IMAGE? why not just reset PKG_IMAGE to
> ZONEROOT?
>
I'm being defensive. If a later change requires changing PKG_IMAGE to
a different value before it is used here, (unlikely I suppose, but I'm
new to this code and want to be conservative) then I wouldn't want to
lose the changed value.
> - looking at this code i'm concerned that it will only work if the gz is
> running the latest bits from the repo. specifically, if "entire" is
> not installed in the gz, then you install SUNWcsd/SUNWcs but you don't
> put any version constraints on those packages. so imagine if you had a
> system running snv_128, then you remove "entire" and add the on-ips
> publishers. then you try to install a zone. then what syncs
> SUNWcsd/SUNWcs with the global zone?
>
> or another example. if a system is already running on-ips nightly
> bits. then the on-ips nightly repo is updated with new bits. then if
> you try to install a zone, you'll happily install bits that aren't
> in sync with the global zone, right?
>
> it seems to me that when installing zones you need to add explicit
> version syncs (which include timestamp granularity) for the following
> packages:
>
> SUNWcsd/SUNWcs
> SUNWips
>
> if you do that, then since those packages depend on their
> incorporations you'll also sync the ON and IPS gate incorporations.
>
> doing this won't sync every ON/IPS package at a timestamp granularity
> (which is ultimately what we need), but it wouldn't be worse that what
> we have today.
>
These questions are (I think) answered by the changes that went into b130
to address suggestion 1 from comment 0 in 12738.
Specifically, I am relying on the fact that now all packages delivered by
a consolidation have a dependency on the consolidation incorporation (to
paraphrase the comment I reference).
However, I'm really relying on the feeback that I have from Liane on the
success of her testing of these changes with the new on ips code that she's
working on.
Perhaps Liane could provide some more detail to this answer?
> ---------
> src/client.py
>
> - ffti: i've never seen this syntax before:
>
> "%(publisher)s\t%(sticky)s\t\ %(preferred)s\t%(enabled)s" % field_dict
>
> and i think it's kinda cool that you can do this with python. but in
> this case the resultant "output = " line is not all that readable and
> doesn't 80 col wrap that well. you could probably improve the
> readability of this code by doing something like:
>
> output = pfx
> if p.sticky:
> output += "\t" + true"
> else:
> output += "\t" + "false"
>
> ed
>
>
> On Wed, Jan 27, 2010 at 12:36:21PM +0000, Gary Pennington wrote:
> > Hi,
> >
> > (I'm hoping to get these changes in b132 to help on-ips work with zones)
> >
> > Webrev:
> >
> > http://cr.opensolaris.org/~garypen/gate/
> >
> > bash-4.0$ hg comment
> > 12738 zone install/attach incorporation logic needs enhancement
> > 14053 pkg brand creation potentially broken with new publisher data
> >
> > The change to client.py is probably most controversial. I've introduced
> > a (very simple) parseable output mode for the publisher sub-command so
> > that I can access the information easily when installing zones.
> >
> > The guiding principles are:
> >
> > - produce output that standard unix text processing tools can process
> > - assume that repetition of redundant data to support line-oriented
> > reporting is fine
> > - don't print headers when using parseable mode
> > - assume that "something better" is coming along in the future and this
> > approach is good enough for now
> >
> > Example outputs:
> >
> > gary...@osol:~$ pkg publisher
> > PUBLISHER TYPE STATUS URI
> > opensolaris.org (preferred) origin online
> > http://pkg.opensolaris.org/dev/
> > opensolaris.org (preferred) mirror online
> > http://pkg-na-2.opensolaris.org/dev/
> > osol (non-sticky, disabled) origin online
> > http://192.168.2.100:10000/
> >
> > gary...@osol:~$ pkg publisher -p
> > opensolaris.org true true true http://pkg.opensolaris.org/dev/
> > origin online
> > opensolaris.org true true true
> > http://pkg-na-2.opensolaris.org/dev/ mirror online
> > osol false false false http://192.168.2.100:10000/ origin
> > online
> >
> > gary...@osol:~$ pkg publisher -pn
> > opensolaris.org true true true http://pkg.opensolaris.org/dev/
> > origin online
> > opensolaris.org true true true
> > http://pkg-na-2.opensolaris.org/dev/ mirror online
> >
> > The changes to common.ksh and pkgcreatezone are less controversial and take
> > advantage of this new output option to fix the two bugs.
> >
> > I'd like to make this change available to everyone using the pkg command,
> > but I would agree if it made more sense to make this option project
> > private and undocumented given the time pressure against the 132 deadline.
> >
> > Gary
> > _______________________________________________
> > pkg-discuss mailing list
> > [email protected]
> > http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
