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. - Should the Identity cert creation go into the Curator? This seems like business logic. - 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. Although the binder is probably a bad name :) - Any reason to not rip out the commented out code blocks for the old methods?
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.
Default Identity Certificate ---------------------------- - nextSerialNumber has a bit of a race condition :) - why is username in the DN?
-- bk
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
----- "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.
- Should the Identity cert creation go into the Curator? This seems
like business logic.
- 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. Although the binder is probably a bad name :)
- Any reason to not rip out the commented out code blocks for the old
methods?
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.
Yeah, I would think so. Is this consistent with how the entitlement certs are being created/persisted? I would think that we want have similar behaviors.
Default Identity Certificate
- nextSerialNumber has a bit of a race condition :)
- why is username in the DN?
This was a request from Pradeep to include it. I didn't really ask any questions. Should we make them issue a separate call to get this?
-- bk _______________________________________________ candlepin mailing list candlepin@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/candlepin
On Fri, Mar 26, 2010 at 3:07 PM, Bryan Kearney bkearney@redhat.com wrote: [snip]
Although the binder is probably a bad name :)
I think binder is a rockin name.
- Any reason to not rip out the commented out code blocks for the old
methods?
removed them
jesus
On Fri, Mar 26, 2010 at 03:07:42PM -0400, Bryan Kearney 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.
Probably not, I just reworked the create method, but I still left it mostly intact. I could probably use even more cleanup like you mention.
- 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. Although the binder is probably a bad name :)
+1 to binder
- Any reason to not rip out the commented out code blocks for the old
methods?
like I said in a previous response, I ripped these out.
jesus
On 03/26/2010 04:19 PM, Jesus Rodriguez wrote:
On Fri, Mar 26, 2010 at 03:07:42PM -0400, Bryan Kearney 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.
Probably not, I just reworked the create method, but I still left it mostly intact. I could probably use even more cleanup like you mention.
Why not? I am just curious on this one.. trying to figure out the role of the curator.
-- bk
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 03/29/2010 10:42 AM, Bryan Kearney wrote:
On 03/26/2010 04:19 PM, Jesus Rodriguez wrote:
On Fri, Mar 26, 2010 at 03:07:42PM -0400, Bryan Kearney 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.
Probably not, I just reworked the create method, but I still left it mostly intact. I could probably use even more cleanup like you mention.
Why not? I am just curious on this one.. trying to figure out the role of the curator.
Cuz that sentence didn't make any sense :) I *think* I was trying to say that it probably was not moved to the curator. I agree it probably SHOULD go there :)
sorry for the confusion.
jesus
- -- jesus m. rodriguez | jesusr@redhat.com principal software engineer | irc: zeus red hat systems management | 919.754.4413 (w) rhce # 805008586930012 | 919.623.0080 (c) +---------------------------------------------+ | "Those who cannot remember the past | | are condemned to repeat it." | | -- George Santayana | +---------------------------------------------+
candlepin@lists.fedorahosted.org