[Freeipa-devel] [PATCH 0184] Always return absolute idnsname in dnszone commands

[Jan Cholasta]

Dne 23.1.2015 v 15:51 Martin Basti napsal(a):
> On 23/01/15 08:22, Jan Cholasta wrote:
>> Dne 20.1.2015 v 12:49 Martin Basti napsal(a):
>>> On 15/01/15 16:07, Jan Cholasta wrote:
>>>> Dne 15.1.2015 v 15:39 Martin Basti napsal(a):
>>>>> On 15/01/15 15:07, Jan Cholasta wrote:
>>>>>> Dne 15.1.2015 v 14:58 Martin Basti napsal(a):
>>>>>>> On 15/01/15 14:25, Jan Cholasta wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Dne 15.1.2015 v 13:27 Martin Basti napsal(a):
>>>>>>>>> On 15/01/15 13:17, Martin Basti wrote:
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4722
>>>>>>>>>>
>>>>>>>>>> Patch attached.
>>>>>>>>>>
>>>>>>>>> Fast fix.
>>>>>>>>>
>>>>>>>>> Updated patch attached.
>>>>>>>>
>>>>>>>> 1) Forward zone commands are not fixed.
>>>>>>> FWzones are new and always normalized to absolute name in ldap
>>>>>>
>>>>>> Would you bet your money on that? Better be safe than sorry,
>>>>>> especially when it's just a matter of moving the code around (right?)
>>>>>>
>>>>>>>>
>>>>>>>> 2) It seems that the primary key returned by -mod, -del and -show
>>>>>>>> (.result.value) is made absolute somewhere else in the code.
>>>>>>>> Would it
>>>>>>>> be possible to do it in one place?
>>>>>>> IMO it is not possible.
>>>>>>>
>>>>>>> Value is generated from key, and key is normalized to absolute zone
>>>>>>> before calling execute.
>>>>>>>
>>>>>>> LDAPUpdate:
>>>>>>> ...
>>>>>>>          if self.obj.primary_key:
>>>>>>>              pkey = keys[-1]
>>>>>>>          else:
>>>>>>>              pkey = None
>>>>>>>
>>>>>>>          return dict(result=entry_attrs, value=pkey_to_value(pkey,
>>>>>>> options))
>>>>>>>
>>>>>>> The idnsname attribute is just taken from LDAP without any
>>>>>>> normalization
>>>>>>
>>>>>> Right.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> 3) Attribute values returned from LDAP are never None, so the if
>>>>>>>> should be "if 'idnsname' in entry_attrs:".
>>>>>>> Ok I will revert the change I made.
>>>>>>>>
>>>>>>>> 4) If idnsname always has only single value, use
>>>>>>>> "entry_attrs.single_value['idnsname'] =
>>>>>>>> entry_attrs.single_value['idnsname'].make_absolute()"
>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>>
>>>>>>> Updated patch attached.
>>>>>>>
>>>>>>
>>>>>>
>>>>> Updated patch attached.
>>>>>
>>>>
>>>> Thanks.
>>>>
>>>> Is there a reason why you put the _make_zone_absolute calls in
>>>> dnszone_* and dnsforwardzone_* instead of DNSZoneBase_*?
>>>>
>>> I moved callback into Base classes.
>>> Patch attached.
>>>
>>
>> 1) Multi-line statements should generally use parentheses for implicit
>> line continuation rather than backslashes:
>>
>> +            entry_attrs.single_value['idnsname'] = (
>> + entry_attrs.single_value['idnsname'].make_absolute())
>>
>> 2) You can remove the DN asserts from dnszone_*, they are already
>> asserted in DNSZoneBase_*.
>>
>> 3) Do not just ignore return values of super().post_callback():
>>
>>     def post_callback(self, something, ...):
>>         something = super(...).post_callback(something, ...)
>>         ...
>>         return something
>>
> Updated patch attached.
>
>

Thanks, ACK.

Pushed to:
master: af0a2409f92e2ef8b322628c0d0569f5e0dac902
ipa-4-1: 270253a999d6a59616634b307f9428e89d1fccb9

-- 
Jan Cholasta