On Fri, Mar 26, 2010 at 4:07 PM, Bryan Kearney bkearney@redhat.com wrote:
I took my own advice.. And reviewed one of the existing stories. I have a bunch of comments below.
Reviewing the Create Consumer Code
ConsumerResource
- I assume the owner creation code in the ctor should be a fixme.
+1
- Should the Identity cert creation go into the Curator? This seems like
business logic.
+1, may as well move it into ConsumerCurator.create.
- In general, many of these classes actually implement business logic
(much of the bind logic). Should this move into a lower level business curator? Perhaps there are other interim curators like the entitler.
+1 yeah I'd say so.
Although the binder is probably a bad name :)
LOL
- Any reason to not rip out the commented out code blocks for the old
methods?
Not that I know of.
IdentityCertAdapter
- Currently, the adapter needs to (1) Generate and (2) Persist the new
certficate in the IdentityCertificate database. Do we want the candlepin engine proper to actually do the persisting? It seems like this is a little bit too much bleed over across the API.
Seems like we've got a confused state here, the cert service adapter is responsible for generating the cert, is it also responsible for storage? I guess we're going the opposite direction, generate only, Candlepin itself will do the storage and retrieval. This will require some small changes to both the identity cert adapters and entitlement cert adapters.
Default Identity Certificate
- nextSerialNumber has a bit of a race condition :)
Yes I was planning to get the serial number generation fixed up for both types of certs, need to add a task for this but have to wait until next sprint.
- why is username in the DN?
Not sure on this one.
Devan