Brock Pytlik wrote:
Shawn Walker wrote:
Brock Pytlik wrote:
line 1028: An api_errors.CertificateError could happen here for a
remote search on an HTTPS server, right?
Doesn't appear that's the case. Should cert validity be being checked
here? How do I check cert validity for a single server? For one the user
has passed in on the command line?
As we discussed earlier on irc today, you should be checking the cert
validity before attempting to perform a transport operation using the
cert information. You can do that using misc.validate_ssl_cert() and
there are examples of how in src/client.py among other places.
--------------------
modules/indexer.py
--------------------
..
lines 164-170: are there any concerns about the size of this file
and memory footprint since the whole file is being read into memory?
Nope. Right now it's fixed at 128M. I plan in a subsequent wad to make
this adjustable via an environment variable or command line argument.
That's rather concerning given our target memory environment of 512MB.
lines 544-545: This should raise an ApiException instead of a
RuntimeError.
It can't raise an ApiException, you're in code common to the server and
client here. Also, the only way to hit this exception is for a caller of
this function, who lives inside the api, to pass in an unknown value. I
think this should remain a reuntime error. I can change it to an assert
if you prefer.
The problem with a RuntimeError or AssertionError is that it doesn't
allow the client to cleanly exit with an appropriate message and it
doesn't allow callers to choose to ignore the error because they can't
tell specifically what error case occurred.
As such, if you can't use an api_error here, you should consider
creating a custom exception and then having the calling client api code
re-raise the exception as one of its own so that the client can exit
cleanly. Even if this case should never happen, our client should not
traceback when encountering known error cases.
--------------------
modules/search_storage.py
--------------------
lines 149-150, 249: should raise an ApiException; not RuntimeError
Can't raise an apiException here, for same reason as above. I will
change these to throw other exceptions and catch and rethrow in the api
code.
See above.
--------------------
modules/server/depot.py
--------------------
line 263: please don't assert; raise a BadRequest error instead and
use cherrypy.log() to log something meaningful
Why log something?
I assume that it's Really Bad(tm) if this fails and the repository
administrator should now? If not, you don't need to log anything.
--------------------
modules/server/repository.py
--------------------
lines 332-337: With this change, this function no longer yields
search results as they are generated. Was this change intentional?
That's not true. This returns a list of generators.
Ah, that wasn't clear. Thanks.
Have you checked modules/server/api.py search() and the web-based
search in the templates provided by the depot to be certain that
everything is still works correctly? I would think this would be a
bump in the version of pkg.server.api at the least.
I'll check. All I need to do is start a repo, point a browser at it and
see what happens when I do a search?
Yes, but don't forget to set your --content-root to your
proto/root_`uname -p`/usr/share/lib/pkg directory.
Also, please make one last sweep to ensure you're not referencing
"authority" or "auth" anywhere; those should both be "publisher" or
"pub" respectively.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
