Re: [Freeipa-devel] [freeipa PR#10] Client-side CSR autogeneration (comment)
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
[Freeipa-devel] [freeipa PR#10] Client-side CSR autogeneration (comment)
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 -- 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
[Freeipa-devel] [freeipa PR#10] Client-side CSR autogeneration (comment)
LiptonB commented on a pull request """ Some tests for the CSR generation functionality have been added to the pull request. """ See the full comment at https://github.com/freeipa/freeipa/pull/10#issuecomment-247174035 -- 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
[Freeipa-devel] [freeipa PR#10] Client-side CSR autogeneration (comment)
LiptonB commented on a pull request """ I've added a commit (Use data_sources option to define which fields are rendered) that simplifies the way we avoid rendering rules whose source data are missing, as discussed here: https://www.redhat.com/archives/freeipa-devel/2016-September/msg00051.html. I prefer this approach to the macros in the original implementation, but I'm leaving it as a separate commit in case you would like to compare them. """ See the full comment at https://github.com/freeipa/freeipa/pull/10#issuecomment-245096157 -- 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
[Freeipa-devel] [freeipa PR#10] Client-side CSR autogeneration (comment)
LiptonB commented on a pull request """ As discussed elsewhere, this script generation is a fairly low-level operation; you have to specify the helper and know how to run the script. Most users will probably want a command that just takes in a private key location and a profile and requests the cert for them. In case you'd like to look at it now, I have an implementation of that in a separate branch, which I'll create a pull request for after this one is complete: https://github.com/freeipa/freeipa/compare/master...LiptonB:local-cert-build """ See the full comment at https://github.com/freeipa/freeipa/pull/10#issuecomment-243811501 -- 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
