[Freeipa-devel] [PATCH] 151 Add --zonemgr validator

[Rob Crittenden]

Martin Kosek wrote:
> On Fri, 2011-10-21 at 11:31 -0400, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On Fri, 2011-10-14 at 14:11 -0400, Rob Crittenden wrote:
>>>> Martin Kosek wrote:
>>>>> Do at least a basic validation of DNS zone manager mail address.
>>>>>
>>>>> Do not require '@' to be in the mail address as it is not used
>>>>> in common DNS zone configuration (in bind for example) and people
>>>>> may be used to configure it that way. '@' is always removed by the
>>>>> installer before the DNS zone is created.
>>>>>
>>>>> https://fedorahosted.org/freeipa/ticket/1966
>>>>
>>>> There is already a zonemgr_callback defined for this option, can the
>>>> verify_zonemgr call be either integrated or called from that?
>>>>
>>>> rob
>>>>
>>>
>>> Right. Please, try this one. I also added a parser error when more than
>>> one '@' is in the checked value.
>>>
>>> Martin
>>
>> A couple of things:
>>
>> In the block where you are counting @ why not add an :
>>
>> else:
>>       raise ValueError('address is not fully qualified')
>>
>> rather than looking for '.' in the result? I think it will be clearer
>> that way. I wonder if the error should contain an example as well, are
>> people going to know what a fully-qualified means?
>>
>> The regex is very strict for e-mail addresses, perhaps too much so. It
>> doesn't allow upper-case characters, periods or _, both of which are
>> allowed in login names. A common e-mail format is first.last at domain.
>>
>> rob
>
> I reorganized the validator a little and let people enter _ in the
> domain name. I also added a small explanation of what we mean by
> fully-qualified.
>
> Since we have the zonemgr validator available, why not use it for the
> DNS plugin too? I enhanced the plugin to use this validator too. Please,
> see attached patch.
>
> Martin

NACK, a client might not have the server sub-package installed so the 
import of bindinstance will fail.

I think that moving the validator into dns.py as a central location 
should work though.

Otherwise looks good.

rob