Resending because this seemed to never reach the list when I sent it
last week.
Brock
On 08/ 5/10 03:17 PM, Danek Duvall wrote:
This is a fairly surface review -- a couple of high-level items and a
bunch
of teeny nits.
Brock Pytlik wrote:
http://cr.opensolaris.org/~bpytlik/ips-11611-v4/
The signature action attributes -- "pkg.sig.X" should just be "X".
Unprefixed attributes are owned by the action, while "pkg." prefixed
attributes are platform-independent, non-action attributes that the
packaging system owns. That'll also allow you to expand some of the
names
so they're more readable -- "alg" -> "algorithm"
Ok
Can a manifest re-opened by an append operation have anything added to it
other than signature actions? Line 109 in server/transaction.py seems to
indicate that you make that distinction, but "non_sig" isn't anywhere
else
in your patch, so perhaps that's dead code?
Thanks, I thought I had written that code, but clearly I haven't. The
only action that should be added to a manifest (currently) is a
signature action. Files (not file actions) can also be added to a
transaction. I'll make sure to add that check in and add a test for it.
signed_manifests.txt:
- line 145: pkg.sig.existing_pkg_vars doesn't appear anywhere in the
code.
Right, that's future work. All variant related issues for signatures are
currently punted down the road except for supporting non-variant tagged
signatures for manifests with variant tagged actions. That is supported.
- line 190, et al: "atter" -> "attr"
Thanks
- line 237: "manfiest" -> "manifest"
Ok
man/pkg.1.txt:
- line 596, et al: underscores instead of spaces when you're naming a
single object, as in previous options
Oops, thanks for catching that.
man/pkgsign.1.txt:
- line 10: it's a bit confusing to have one or more packages
specified by
operands, but all packages specified by an option. Perhaps remove
--sign-all and accept "*" as an argument representing all packages?
Dan suggested "all" as a special keyword. The reason I think --sign-all
is the right answer is because it's just the first of several options
I'd like to add along those lines. I think --sign-newest and
--sign-unsigned will be two much more commonly used options. I chose to
put those off for now because they weren't needed for our process. Given
that, I prefer to stick with --sign-all over "all" or "*".
- line 27: "manfiest" -> "manifest"
Thanks
- line 31: "verifing" -> "verifying"
Thanks
- line 42: this downloads and re-publishes, correct? It might be
useful
to explain why you'd want to do this, particularly since you're
moving
a lot of data around this way. Does this publish at a new
timestamp?
Bleh, documentation for that option is just totally wrong. -s serves the
same function that it does in pkgrecv or pkgsend. To be clear, pkgsign
never republishes a package (meaning the pkg's timestamp changed), it
always modifies an existing manifest.
It's now:
With -s, sign packages in the repository at the given URI.
- line 50: is the catalog changed by adding a signature action? I
didn't
see any changes to the catalog code, and I don't see signature
actions
referred to in the existing catalog code.
Signature actions are not stored as part of the catalog data. However,
adding a signature to a manifest does change the manifest's hash, so the
catalog must be updated. The replace_package method (called from
accept_append) replaces the old hash value in the catalog with the new
hash.
actions/generic.py:
- line 82: instead of "has_data_for_publication", how about
"payload" or
"has_payload"? "payladen"?
Sure, I can switch to has_payload if that's preferable.
manifest.py:
- line 415: unnecessary
k
publish/transaction.py:
- line 153, 266: "additional"
Thanks
server/transaction.py:
- line 265: what's XXX here, and why aren't you doing it in __init__?
I don't know. open (line 156) which append strongly resembles, had this
line, so I retained it.
- line 483: pkg.chain_certs.sizes doesn't seem to be used anywhere
else.
Thanks, I missed converting that when I switched from
pkg.chain_certs.sizes to pkg.chain.sizes.
package%2Fpkg.p5m:
- line 262: the first dash in the name should be a slash
Thanks for taking a look at the changes,
Brock
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
