On 06/27/10 08:07 PM, Brock Pytlik wrote:
Webrev:
http://cr.opensolaris.org/~bpytlik/ips-11611-v1/
I've skipped over the configuration stuff since we already discussed
that. I'm also ignoring general formatting since I know this isn't the
final webrev.
src/client.py:
line 742:
So I understand the conditional here is because no Action
is being returned, but what about changing image.py to
instead return the signature action that was being used
at the point of validation failure? I realise that in
some cases there won't be one, but it seems useful to
return it if possible.
src/modules/client/publisher.py:
lines 1788-1790:
This can (optionally) be simplified as:
with open(pkg_hsh_pth, "wb") as fh:
fh.write(s)
...python 2.6's contextmanager will automatically close
the file when execution exits the block even if an exception
happens. This exists in other places too (lines 1836-1839,
2012-2029).
line 1794: this needs exception handling
line 1811: this needs exception handling
line 1845:
I'm sure this is just for debugging, but this should be an
ApiException class instead of RuntimeError().
line 1865:
EROFS, EACCES, etc. should be re-raised as ApiException equivalent
line 1867:
pass isn't needed since statements exist; was this intended to be
"return" ?
line 1902: s/are set/are set to/
src/modules/server/depot.py:
As we discussed, I'd like to see the following changes made:
* get rid of close_append here and possibly in repository.py;
instead track the type of transaction and in server/transaction
close() call the appropriate method to close the transaction
based on the type
* instead of having add_cert as its own operation, see if you can
add a new version of the /file/ operation, rename add_cert to
file_1, and have cherrypy use that to handle uploads. Be
certain that it requires a Transaction ID header be present.
src/modules/server/repository.py:
Consider renaming add_cert to add_file since it's clear that this
operation is generic in nature (plus we can reuse this logic
later on for upcoming publication changes).
I can't dig deeper into this at the moment, but I've looked over almost
everything (at least skimming).
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
