[Freeipa-devel] [PATCH 0016] Add RADIUS proxy support to ipalib CLI

[Nathaniel McCallum]

Everything looks good to me. +1

On Thu, 2013-11-28 at 12:18 +0100, Petr Viktorin wrote:
> Thanks!
> Just a bit of cleaning up now, sending a patch with proposed changes to 
> speed things up.
> 
> Patch needs a tiny rebase.
> Points I missed:
> - There are some unused imports.
> - ValidationError takes the attribute name in `name` rather than the 
> name of the CLI option.
> 
> >> Now the validation is too strict, a port is not accepted.
> >
> > Fixed.
> 
> "invalid!" is pretty bad for an error message. I put it in as a 
> placeholder, but I wasn't clear about that, sorry!
> 
> >> Should non-FQDN hostnames be allowed?
> >
> > I agree they should not. Fixed.
> 
> validate_hostname() has a check_fqdn argument, no need to do this manually.
> 
> >>>> ipatokenusermapattribute is also not validated. Not sure if it needs to be.
> >>>
> >>> I don't think validation is really possible outside of the permitted
> >>> characters for an LDAP attribute.
> >>
> >> I think if "$%^&*" is allowed, we'll get a bug from QA soon enough.
> >
> > Fixed.
> 
> The `sre` module is named `re` since Python 2.5.
> 
> >> We generally output lists; this should also be a list with one element.
> >
> > Fixed.
> >
> >> Attaching updated tests.
> >
> > A few of these tests are still failing for me, but it is not immediately
> > obvious why. They seem to be getting answers from previous queries. I'm
> > not sure if this is something wrong with my code or the tests. Can you
> > take a look at it?
> 
> My bad, I've used a wrong variable name.
>