[Freeipa-devel] [freeipa PR#10] Client-side CSR autogeneration (comment)

[Ben Lipton]

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 https://github.com/freeipa/freeipa/pull/10#issuecomment-247244120
>
>
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
    like|certrequest-get-data|(or|certrequest-get-script|).
 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?


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160915/110b791e/attachment.htm>