On 08/ 4/10 09:25 PM, Brock Pytlik wrote:
Greetings all,
Here's the webrev for the manifest signing changes:
http://cr.opensolaris.org/~bpytlik/ips-11611-v4/
doc/client_api_versions.txt:
============================================================
line 4: nit: s/. This/. This/
doc/signed_manifests.txt
============================================================
line 59: nit: s/, exists/ exists/
line 59: suggest: s/purposes. This/purposes and/
line 62: s/It's/Its/
line 138: s/have added/have been added/ ?
line 148: nit: s/take/consider/
line 235: s/inplace/in place/ ?
src/client.py:
============================================================
lines 189-195, 2418-2426: These should probably be consistent with
other options. For example:
--approve-ca-cert=path_to_ca
--revoke-ca-cert=ca_hash
line 2520: nit: s/. %s was set/; %s was set/
lines 2517, 2528, 2536, 3587: s/given:/given: /
line 2915: use os.path.normpath(os.path.join(orig_cwd, ca)) instead;
abspath() uses cwd which is likely different at this point then
where the user started
lines 2915-2918: simplify to this?
ca = os.path.normpath(os.path.join(orig_cwd, ca))
with open(ca, "rb") as fh:
s = fh.read()
lines 3947, 3965: nit: oddly indented relative to rest of list
src/man/pkg.1.txt:
============================================================
lines 59-65: as mentioned before, should probably match other
options by using '--option=value_name'
line 775: s/the stricter/the stricter/
lines 802-803: suggest rewording as:
The pathname of the directory that contains the trust anchors for
the image.
src/man/pkgrepo.1.txt:
============================================================
lines 53-60: s/path/pathname/ ?
line 153: s/certificates/certificate/
lines 153, 158: s/repo in/repository located at/
src/man/pkgsign.1.txt:
line 53: suggest: s/sign all the/sign all of the/ ?
src/modules/actions/generic.py:
line 81: s/Most types/Most types of/ ?
src/modules/actions/signature.py:
lines 52-53: The hasattr doesn't make sense given that hash is
declared on line 43. Do you know why this was needed? Same
for lines 54-55.
lines 94-97: Since you do an os.stat on path anyway on line 104,
it seems logical to do the stat once at the beginning and then
use stat.S_IS* checks on the result to determine whether to
raise exceptions, etc.
lines 108-112: Except it doesn't close() it automatically anymore
since you provided a file-like object instead of the pathname of
a file. So this needs to close the file object itself now.
line 117: Why not just " ".join(sizes) ?
line 153: keep in mind get_data_digest() no longer closes the file
object automatically
lines 232-237: nits: Blank lines between parameter descriptions
appreciated. The general convention for parameter names
is to use single quotes, not double quotes in the docstring.
This is in other docstrings too.
line 240: s/if the version/and the version/ ?
line 264: nit: s/action:%s/action: %s/
line 293: nit: s/Res:%s/Res: %s/
src/modules/client/api.py:
============================================================
lines 2288-2290: I don't really understand the need to revoke and
unset CAs during the *initial* addition of a publisher to an image.
At the very least, the unset seems odd. Can you expound?
src/modules/client/api_errors.py:
============================================================
line 1395, 1401, 1512, 1536, 1559: nit: the else: really isn't needed
since you return for the previous case right? Would make some
of the text easier to read since you could lose the indent.
lines 1577-1578: odd line wrapping
src/modules/client/imageconfig.py:
============================================================
line 738: s/properites/properties/
line 752: s/Accesor/Accessor/
src/modules/client/publisher.py:
============================================================
Nit: Our usual convention for docstrings is to use single quotes for
naming parameters instead of double quotes.
line 1871: why is this one 'hsh' while the others are 'hash'?
line 1926: Do you really want this, or did you want "if self.ca_dict
is not None" ?
line 1979: nit: s/is:%(name)s/is: %(name)s/
line 2041: s/in // ?
line 2276: extra newline?
src/modules/client/transport/repo.py:
============================================================
line 1357: missing docstring
lines 1999-2000: what action?
src/modules/manifest.py:
============================================================
line 415: why the explicit return? it's unnecessary
src/modules/misc.py:
============================================================
lines 424-429: The line wrapping seems odd here.
src/modules/server/depot.py:
============================================================
line 724: s/add/file/
line 954: s/certificate/file/
line 969: An empty file should be allowed.
lines 982-985: You shouldn't need to set these. Did the
decorator on lines 763-765 not work for this case?
src/modules/server/repository.py:
============================================================
line 1371: bare raise followed by another raise?
lines 1559-1560: "to the as" ?
lines 1586-1587: commented code
src/modules/server/transaction.py
============================================================
line 535: s/certificate/file/
src/tests/pkg5unittest.py:
============================================================
lines 1382-1412: You should be able to implement this in terms of
using self.cmdline_run just as "def pkgrepo" and others are done.
That avoids the duplication of handling the env, subprocess, etc.
src/sign.py:
============================================================
line 99: commented code
line 128: s/pkgsign/pkg/ as Rich Lowe pointed out recently
I apologise in advance if someone else has already mentioned these.
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
