On 09/15/2016 02:12 AM, jcholast wrote:
jcholast commented on a pull request

In addition to my inline comments above:

1. "Certificate mapping" does not really evoke "certificate request templating" 
to me, and is also used in the context of mapping identities to certificates. Could we use a more 
suitable name to avoid confusion?
2. The `ipalib.certmapping` module is used only in `ipaclient`, so that's where 
it should be located. It can be moved to `ipalib` later if necessary.
3. I don't think `IPAExtension` deserves it's own module, at least not now.

See the full comment at 

Tried sending my comments as a "review" (new Github feature) and it seems they don't get sent to the list that way. So:

Thanks for the comments! I've fixed the simple ones and replied to the rest. Regarding your comments about file organization:

1. I quite agree that certmapping isn't a good name for what this
   turned out to be. With the convention of naming modules after the
   objects they model, perhaps a good name would
   be|certrequest|or|csr|? The command could be renamed to something
2. Just to confirm, you're suggesting just moving these classes to
   the|ipaclient.plugins.<whatever we end up calling it>|module?
3. Seems reasonable, I've moved it into the ipalib module for now. It
   will go wherever the contents of that module end up.

Logistical stuff:

 * Now that this is under review I won't add any more content. Are you
   ok with the two commits about testing being part of this review or
   should I remove them?
 * If you run rebase --autosquash with the latest commit it doesn't
   actually apply cleanly, but I'm trying not to change history while
   it's being reviewed, so I'll do the rebase later on if that's ok?

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to