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

2016-06-29 Thread Jan Cholasta 

On 29.6.2016 10:14, Stanislav Laznicka wrote:

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.


Thanks, ACK.

Pushed to master: 427bbf6c0d61cf52f14a9f2606143f994340ec83

--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-29 Thread 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.
From 29c4db5e6696ae1885a458914fad3f251b0a4e8a Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.
Also had to fix some tests as objectClass will not be returned by
default now.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py  | 4 ++--
 ipatests/test_xmlrpc/test_permission_plugin.py | 4 
 ipatests/test_xmlrpc/test_role_plugin.py   | 5 -
 3 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..c35f660c790290aa0449ea2441a4fd0f5baed880 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2159,7 +2159,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list)
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
@@ -2258,7 +2258,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list)
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
diff --git

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

2016-06-28 Thread Stanislav Laznicka 

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.


From 821960d8e84f2f4e075af93321398ea62af2e206 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.
Also had to fix some tests as objectClass will not be returned by
default now.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py  | 8 ++--
 ipatests/test_xmlrpc/test_permission_plugin.py | 4 
 ipatests/test_xmlrpc/test_role_plugin.py   | 5 -
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..6fc56e0f8218a2ace30bb858b91b365490b5d051 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2126,6 +2126,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 result = self.api.Command[self.show_command](keys[-1])['result']
 dn = result['dn']
 assert isinstance(dn, DN)
+entry_attrs = ldap.make_entry(dn, self.args_options_2_entry(**options))
 
 for callback in self.get_callbacks('pre'):
 dn = callback(self, ldap, dn, *keys, **options)
@@ -2135,6 +2136,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 attrs_list = ['*'] + self.obj.default_attributes
 else:
 attrs_list = set(self.obj.default_attributes)
+attrs_list.update(entry_attrs.keys())
 if options.get('no_members', False):
 attrs_list.difference_update(self.obj.attribute_members)
 attrs_list = list(attrs_list)
@@ -2159,7 +2161,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs =

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

2016-06-17 Thread Stanislav Laznicka 

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?


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-14 Thread Jan Cholasta 

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.)


The first one is revert the patch that removes attr_list, and was pushed.


That does not make it any more special than any other commit.





My 2c
Martin^2








--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-14 Thread Martin Basti 



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.)


The first one is revert the patch that removes attr_list, and was pushed.



My 2c
Martin^2





--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-14 Thread 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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-14 Thread Martin Basti 



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


My 2c
Martin^2

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-08 Thread Stanislav Laznicka 

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)?
From d554d96135160c8adcd3c798876de1e5ae83b4a0 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 12:11:39 +0200
Subject: [PATCH 1/2] Revert "Removed dead code from
 LDAP{Remove,Add}ReverseMember"

While the code was really dead, it should serve a purpose elsewhere.
This reverts commit c56d65b064e1e0410c03cf1206816cad4d8d86cc.
---
 ipaserver/plugins/baseldap.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 7367c87988bfa6f1db5c38c6dd633e541f59d27e..62b726da1a0baef9fc9fa4bb386a2101ca1f10b2 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2131,6 +2131,14 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
+if options.get('all', False):
+attrs_list = ['*'] + self.obj.default_attributes
+else:
+attrs_list = set(self.obj.default_attributes)
+if options.get('no_members', False):
+attrs_list.difference_update(self.obj.attribute_members)
+attrs_list = list(attrs_list)
+
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
@@ -,6 +2230,14 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
+if options.get('all', False):
+attrs_list = ['*'] + self.obj.default_attributes
+else:
+attrs_list = set(self.obj.default_attributes)
+if options.get('no_members', False):
+attrs_list.difference_update(self.obj.attribute_members)
+attrs_list = list(attrs_list)
+
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

From 5a3c58f27efe73bcf478051d502df246ed10d0d2 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH 2/2] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..22eea887c4425bf890e0dc0da9e8779812fe5769 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2159,7 +2159,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
 # Update the member data.
-entry_attrs = ldap.get_entry(dn, ['*'])
+entry_attrs = ldap.get_entry(dn, attrs_list + ['objectclass'])
 self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
 for callback in self.get_callbacks('post'):
@@ -2258,7 +2258,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 failed['member'][self.reverse_attr].append((attr, unicode(e)))

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

2016-06-07 Thread Martin Basti 



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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-07 Thread Jan Cholasta 

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.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-07 Thread Martin Basti 



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?

Martin^2

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-07 Thread Jan Cholasta 

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.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-06 Thread Martin Basti 



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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-03 Thread Stanislav Laznicka 

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

From 6bd0ea9cb7749cceba40e5df03aeca2e9ee3b70d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 3 Jun 2016 14:08:59 +0200
Subject: [PATCH] Removed dead code from LDAP{Remove,Add}ReverseMember

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 16 
 1 file changed, 16 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index bbd8ba146ead81857bbc4c2aee550b855b846be5..3f0e68b143f2dcb50be6bc49a752b19da02a248d 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2129,14 +2129,6 @@ class LDAPAddReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
-if options.get('all', False):
-attrs_list = ['*'] + self.obj.default_attributes
-else:
-attrs_list = set(self.obj.default_attributes)
-if options.get('no_members', False):
-attrs_list.difference_update(self.obj.attribute_members)
-attrs_list = list(attrs_list)
-
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
@@ -2228,14 +2220,6 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
-if options.get('all', False):
-attrs_list = ['*'] + self.obj.default_attributes
-else:
-attrs_list = set(self.obj.default_attributes)
-if options.get('no_members', False):
-attrs_list.difference_update(self.obj.attribute_members)
-attrs_list = list(attrs_list)
-
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-03 Thread Martin Basti 



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
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

2016-06-03 Thread Stanislav Laznicka 

https://fedorahosted.org/freeipa/ticket/5892

From 350cfa89f34a6f9beddc85a195963966e1aa561d Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka 
Date: Fri, 3 Jun 2016 14:08:59 +0200
Subject: [PATCH] Removed dead code from LDAPRemoveReverseMember

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py | 8 
 1 file changed, 8 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index bbd8ba146ead81857bbc4c2aee550b855b846be5..f60fb505961ed3b6ce97b505623af153532dd1c9 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2228,14 +2228,6 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
 dn = callback(self, ldap, dn, *keys, **options)
 assert isinstance(dn, DN)
 
-if options.get('all', False):
-attrs_list = ['*'] + self.obj.default_attributes
-else:
-attrs_list = set(self.obj.default_attributes)
-if options.get('no_members', False):
-attrs_list.difference_update(self.obj.attribute_members)
-attrs_list = list(attrs_list)
-
 completed = 0
 failed = {'member': {self.reverse_attr: []}}
 for attr in options.get(self.reverse_attr) or []:
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code