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

[Rob Crittenden]

Martin Kosek wrote:
> On Mon, 2011-10-24 at 17:08 +0200, Martin Kosek wrote:
>> On Mon, 2011-10-24 at 09:02 -0400, Rob Crittenden wrote:
>>> 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
>>
>> Thanks. I didn't realize that user can have freeipa-admintools without
>> freeipa-server installed.
>>
>> I placed the validator to ipalib/util.py as we cannot place it to the
>> dns plugin directly as all our plugins assume that we have initialized
>> api.
>>
>> Updated patch attached.
>>
>> Martin
>
> And now also with API.txt change. This patch must be cursed or
> something.
>
> No need to bump API version, just the validator has been added.
>
> Martin

ACK