[Freeipa-devel] [PATCH 0042] Removed dead code from LDAPRemoveReverseMember

[Jan Cholasta]

On 14.6.2016 16:29, Martin Basti wrote:
>
>
> On 08.06.2016 14:17, Stanislav Laznicka wrote:
>> On 06/07/2016 10:42 AM, Martin Basti wrote:
>>>
>>>
>>> On 07.06.2016 10:43, Jan Cholasta wrote:
>>>> On 7.6.2016 10:22, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 07.06.2016 09:07, Jan Cholasta wrote:
>>>>>> On 6.6.2016 18:29, Martin Basti wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 03.06.2016 14:28, Stanislav Laznicka wrote:
>>>>>>>> On 06/03/2016 02:19 PM, Martin Basti wrote:
>>>>>>>>>
>>>>>>>>> On 03.06.2016 14:13, Stanislav Laznicka wrote:
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/5892
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>> NACK
>>>>>>>>>
>>>>>>>>> please remove it from LDAPAddReverseMember too, it contains the
>>>>>>>>> same
>>>>>>>>> code
>>>>>>>>>
>>>>>>>>> Martin^2
>>>>>>>>
>>>>>>>> Please see the modified patch.
>>>>>>>>
>>>>>>>> Standa
>>>>>>>>
>>>>>>> ACK
>>>>>>>
>>>>>>> Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc
>>>>>>
>>>>>> I think the attrs_list was supposed to be passed to the
>>>>>> ldap.get_entry() call rather than removed, which would fix that every
>>>>>> reverse member command always acts like --all was specified.
>>>>>>
>>>>> I'm really afraid, what can happen if we put attr_list into
>>>>> get_entry()
>>>>> instead of '*', because this code were there for 4 years and I don't
>>>>> feel happy enough to change it now, what we may break.
>>>>>
>>>>> Should I revert this commit then and postpone the ticket?
>>>>
>>>> It's a bug and should be fixed. The fix is easy so I see no point in
>>>> postponing it. I see no reason to be really afraid, I'm pretty sure
>>>> that removing the objectclass attribute (which is invisible in the
>>>> CLI anyway) from the output of all the 4 commands that use this code
>>>> won't break anything.
>>>>
>>>
>>> Ok
>> It seems that tests expect objectClass to be always returned in
>> derived methods. Is that expected behavior? If so, please see the
>> attached patch. I wonder if the keys of the passed options should make
>> it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?
>
> I still don't think that we should use that attrs list, because
> according git history, attrs_list was really used only with code that
> was removed, and as you can see Standa had add extra objectclass
> attribute there

So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)

>
> My 2c
> Martin^2


-- 
Jan Cholasta