[email protected] wrote:
On Mon, Aug 31, 2009 at 08:55:03PM -0500, Shawn Walker wrote:
webrev:
http://cr.opensolaris.org/~swalker/pkg-cat-p2/
catalog.py:
- JSONWriter has no docstrings. It would be great if you could add
some description about what this class and its methods do.
It is a private helper class for serialisation, I've added several
docstrings to make that clearer and prefixed its name with '_' (can't
use '__' for the class name).
- line 61 & 112: It looks like you don't remove the temporary file if
everything goes according to plan? Is this intentional, and if so,
who cleans up the tempfiles when you're done?
The idea is that the JSONWriter creates a temporary file to store the
serialized results of the data provided by the caller. Once the
serialization is complete, the caller renames the file into place
wherever they want it using json_writer_obj.pathname. Since this is a
private helper class, that seems reasonable.
- line 228 & 237: Why are you raising a RetrievalError here? It
looks to me like you're loading something off disk, so it would make
sense to raise CatalogNotFound, or similar, instead. My expectation
would be that if we're unable to retrieve the catalog during
transport, the exception for the retrieval failure would be
generated from the transport code.
RetrievalError is not a transport-system specific error. It's a generic
retrieval error class that I use with the p5i class and other places as
well. It simply says the data can't be accessed at the requested
location. I didn't see the need for a Catalog specific exception here.
The exception class could be better named likely.
- line 305: Can you explain why you used super() here instead of
CatalogPartBase? All of the other code that I've seen uses the name
of the parent class explicitly.
In case there are further sub-classes in the future, this allows those
to work correctly. super() handles certain patterns of inheritance that
just simply using the base class name would not.
pkgplan.py:
- lines 81, 101, 108: Would you replace these with something other
than a RuntimeError. IIRC, Danek asked that we avoid raising that
exception. Since you're modifying this code, let's have it raise
something sane.
I've changed these to raise a PlanCreationException in addition to
adding a 'installed' and 'not_installed' error case to the exception class.
manifest.py:
- Who performs the validation of these manifests' signatures? In
No one yet; this is just plumbing.
particular, it doesn't look like we take any action beyond raising
an exception if verification fails. Bug 6011 details the need to
re-download corrupt manifests. If we're able to determine that a
manifest isn't valid, the transport needs to know that it encoutered
an error while downloading the manifest, and we should re-download
the manifest's content; however, I don't see either of these things
happening here. Is this part of Phase III?
Yes and no. For the client to get the manifest signature data, the
server has to provide v1 catalogs. So the completion of the plumbing
work will be done in Phase III, but I will be opening a bug for another
enterprising soul to take advantage of the new aqueduct :)
General comments:
- You mentioned that this is going to represent a flag day for
clients. Is there anything that RE needs to do to ensure a smooth
transition here?
Not for Phase II; Phase III will likely require some finagling.
- I'm also curious if there's anything that needs to happen on the
deployment side of things. My guess is that it's not an issue for
the short term, but are there any code changes coming that are going
to cause problems if the depot software isn't upgraded promptly?
Phase III will require some RE/ipkg work. But from the end-user's
point, the full intent (at least for this release cycle) is for clients
to be able to use depot servers that only provide v0 catalogs with impunity.
Cheers,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
