On 07/ 8/10 05:48 PM, Shawn Walker wrote:
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.
Ok, I've changed things in image.py so that it'll populate the act field
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).
Ah, excellent, I'll switch to that construction.
line 1794: this needs exception handling
So you're worried about the case where 4 lines before, I could create a
file but now I can't remove it? Ok. Would you prefer the Environment
error on removal to surface and hide that there was a file format error
or should the failure to remove the file be hidden and that the file was
in a bad format be shown to the user?
line 1811: this needs exception handling
Again, so in this situation, I was able to create all the files and
directories I needed, but I couldn't create a symlink? Ok. I've added
something to catch a permissions error and raise the api version of
that. Are there other error conditions you're concerned about?
line 1845:
I'm sure this is just for debugging, but this should be an
ApiException class instead of RuntimeError().
Thanks for catching that, I'd forgotten I hadn't converted that over.
line 1865:
EROFS, EACCES, etc. should be re-raised as ApiException equivalent
Ok
line 1867:
pass isn't needed since statements exist; was this intended to be
"return" ?
Nope, just an extraneous pass.
line 1902: s/are set/are set to/
Thanks
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
Done
* 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.
Ok
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).
Sure.
I can't dig deeper into this at the moment, but I've looked over
almost everything (at least skimming).
Thanks for taking a look.
Brock
Cheers,
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
