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.|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:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
