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

[Stanislav Laznicka]

On 06/28/2016 10:34 AM, Stanislav Laznicka wrote:
> On 06/17/2016 09:14 AM, Stanislav Laznicka wrote:
>> On 06/14/2016 04:40 PM, Jan Cholasta wrote:
>>> On 14.6.2016 16:35, Martin Basti wrote:
>>>> On 14.06.2016 16:37, Jan Cholasta wrote:
>>>>> 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.)
>>>>>
>> Right. I added the objectclass so that it behaves similar to what the 
>> other commands do and so that it does not break tests but it can be 
>> removed and tests could be fixed. The question here is - do we want 
>> it in 4.4, then? If so can we be sure it won't break anything 
>> (although we know that it's broken as it is, and it's been like that 
>> for the past 4 years)?
>>
>> Currently, the LDAP*ReverseMember are used in adding/removing 
>> permissions/privileges to privileges/roles so I think we don't want 
>> it to go bad - but could it?
>>
> After discussion with Honza we agreed that the patch should not break 
> anything while trying to fix the bug. Attached is the new patch with 
> fixed tests.
>
>
Hopefully last modification - attrs_list should not be updated by 
received options as these don't reflect the entry's actual attributes.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0045-3-The-LDAP-ReverseMember-shouldn-t-imply-all-is-always.patch
Type: text/x-patch
Size: 5160 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160629/bf864bca/attachment.bin>