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