On 23.8.2016 11:46, Fraser Tweedale wrote:
Thanks for review; rebased and updated patch attached. Only 0090
has substantive changes.
Cheers,
Fraser
On Mon, Aug 22, 2016 at 09:22:08AM +0200, Jan Cholasta wrote:
On 19.8.2016 13:11, Fraser Tweedale wrote:
Bump for review.
On Mon, Aug 15, 2016 at 05:15:16PM +1000, Fraser Tweedale wrote:
Thanks for reviews. Rebased and updated patches attached (and one
new patch). No substantive changes to 92..94. Patch order is:
92-2, 93-2, 94-2, 98, 90-3
Other comments inline.
Thanks,
Fraser
On Fri, Aug 12, 2016 at 11:33:28AM +0200, Jan Cholasta wrote:
Patch 0092: ACK
Patch 0093: ACK
Patch 0094: ACK
Please fix this PEP8 issue before pushing:
./ipaserver/plugins/cert.py:597:17: W503 line break before binary operator
Patch 0098: ACK
Patch 0090:
1) Generic otherNames (san_other) do not work correctly. The OID is not
included in the value and names with complex type other than
KerberosPrincipal are not parsed correctly. The value should include the OID
and DER blob of the name.
Updated to use "OID:b64(DER)" as the attribute value.
OK.
2) With --all, san_other should be included in the result for all
otherNames, even the known ones, to provide (limited) forward compatibility.
Done; when --all given, known otherName kinds are included in
'san_other' attribute in addition to their own attribute.
OK.
3) Do we have to support *all* the name types? I mean we could, for the sake
of completeness, but it might be easier to just keep the few ones we
actually care about (email, DNS name, principal name, UPN and directory name
in your patch 0095).
Yeah, why not support them all? See also Petr's comments in other
branch of thread.
Works for me, but see Lukáš's reply, I think he has a point. Maybe we can
make a compromise and show only supported name types by default and
everything with --all?
Now only showing DNSName, RFC822Name, DirectoryName, UPN and
KRBPrincipalName unless --all is given.
4)
+ obj.setdefault(attr_name, []).append(unicode(name))
The value should not (always) be unicode, but of the type declared by the
param (unicode or ipapython.kerberos.Principal or
ipapython.dnsutil.DNSName).
I now pass the value to the constructor of whatever type the
parameter uses:
attr_value = self.params[attr_name].type(name_formatted)
obj.setdefault(attr_name, []).append(attr_value)
OK.
5) san_directoryname should be a DNParam rather than Str.
Fixed, thanks.
6) Could we use "Subject <name type>" instead of "Subject Alternative Name
(<name type>)" for labels? Or something else which is shorter and has the
name type more "visible" than the current form.
No worries.
7) The patch needs a rebase.
Thanks, ACK, but I have a couple additional nitpicks:
8) I think we should move the san_* param definitions right after
subject param definition, so that they are visually close in CLI output.
9) san_directoryname should be added to default_attrs in patch 95, not here.
I took the liberty of fixing these myself.
Pushed to master: 48aaf2bbf5df6dcedaa466753c8fafb478cb31b2
Honza
--
Jan Cholasta
--
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
