[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (opened)

[fidencio]

celestian's pull request #13: "MEMBEROF: Don't resolve members if they are removed" was opened

PR body:
"""
## TOPIC
If we need remove ghost (SYSDB_GHOST, DB_GHOST) attribute
from group we use empty structure.

This doesn't mean that there is pointer to NULL but
it means that there is zero elements.
Ghost attribute is array not string.

Resolves:
https://fedorahosted.org/sssd/ticket/2940

## REPRODUCER

preparing:
```
ipa user-add --first=Adam --last=Adam --email=[e-mail] adam
ipa group-add group_1
ipa group-add-member --users=adam group_1
ipa group-add group_2
```
reproducer (possible repeat):
```
systemctl daemon-reload
sudo su -c "truncate -s0 /var/log/sssd/*.log"
sudo su -c "rm -f /var/lib/sss/db/*" 
sudo su -c "rm -f /var/lib/sss/mc/*"
sudo systemctl restart sssd.service

ipa group-add-member --groups=group_1 group_2
sss_cache -UG
sudo su -c "truncate -s0 /var/log/sssd/*.log"
getent group group_2

ipa group-remove-member --groups=group_1 group_2
sss_cache -UG
sudo su -c "truncate -s0 /var/log/sssd/*.log"
getent group group_2
```

"""

See the full pull-request at https://github.com/SSSD/sssd/pull/13
... or pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/13/head:pr13
git checkout pr13

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

celestian commented on a pull request

"""
Note: Please, which type of our CI is the best for tests?
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244375070

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

mbasti-rh commented on a pull request

"""
IMO you can use `mod_ctx->membel == NULL` only once

```
if (mod_ctx->membel == NULL && (
    mod_ctx->ghel == NULL ||
       (mod_ctx->ghel != NULL &&  mod_ctx->ghel->num_values == 0)
))
```

Just saying, mainly I want to test utf-8 notifications :)
Martin Bašti
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244396922

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

fidencio commented on a pull request

"""
Sorry for the noise in this PR, but I'm just testing whether my surname will break Martin's script again or not.

Fabiano Fidêncio
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244397795

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

fidencio commented on a pull request

"""
Sorry for the noise in this PR, but I'm just testing whether my surname will break Martin's script again or not.

Fabiano Fidêncio
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244397795

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

fidencio commented on a pull request

"""
I basically agree with Martin here.

IMO the code could be a bit simplified a little bit more, something like:
`if (mod_ctx->membel == NULL &&
    (mod_ctx->ghel == NULL || mod_ctx->ghel->num_values == 0))`

Fabiano Fidêncio
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244403499

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

fidencio commented on a pull request

"""
I basically agree with Martin here.

IMO the code could be simplified a little bit more, something like:
`if (mod_ctx->membel == NULL &&
    (mod_ctx->ghel == NULL || mod_ctx->ghel->num_values == 0))`

Fabiano Fidêncio
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244403499

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

celestian commented on a pull request

"""
I wrote the conditions in fully way for clarity. Yes, it is possible to write it in condensed way.

My question in note is about which type of our CI I can use. It is in memberof plugin. I don't see any test for memberof plugin at all.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244409956

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

lslebodn commented on a pull request

"""
On (02/09/16 08:37), celestian wrote:
> I wrote the conditions in fully way for clarity. Yes, it is possible to write it in condensed way.
>
> My question in note is about which type of our CI I can use. It is in memberof plugin. I don't see any test for memberof plugin at all.

IIRC some parts of memberof pluging are testin in unit test
src/tests/sysdb-tests

But it should be possible to write integration tests as well.
It is not a IPA specific bug.

Or what did you mean by "which type of our CI I can use"?

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244454876

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (comment)

[fidencio]

jhrozek commented on a pull request

"""
On Fri, Sep 02, 2016 at 08:37:07AM -0700, celestian wrote:
> I wrote the conditions in fully way for clarity. Yes, it is possible to write it in condensed way.
>
> My question in note is about which type of our CI I can use. It is in memberof plugin. I don't see any test for memberof plugin at all.

Look into src/tests/sysdb-tests.c, search for "tc_memberof". There is
already quite a few tests for the memberof plugin.

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-244928996

[sssd PR#13] MEMBEROF: Don't resolve members if they are removed (+nack)

[fidencio]

celestian's pull request #13: "MEMBEROF: Don't resolve members if they are removed" label *nack* has been added

See the full pull-request at https://github.com/SSSD/sssd/pull/13

[sssd PR#13][synchronized] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Author: celestian
 Title: #13: MEMBEROF: Don't resolve members if they are removed
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/13/head:pr13
git checkout pr13

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
Hello,
I pushed new version with tests.

Thanks @sumit-bose with help with the second patch. By the way is there better way how to write co-authors? Please tell me.

This patch works for ipa provider, not for ldap provider. (For ldap provider we have https://fedorahosted.org/sssd/ticket/3186)

Reproducer:
```bash
# !/bin/bash

# prepare
ipa user-add --first=Adam --last=Adam --email=[e-mail] adam
ipa group-add group_1
ipa group-add-member --users=adam group_1
ipa group-add group_2

# reproducer

systemctl daemon-reload
sudo su -c "truncate -s0 /var/log/sssd/*.log"
sudo su -c "rm -f /var/lib/sss/db/*" 
sudo su -c "rm -f /var/lib/sss/mc/*"
sudo systemctl restart sssd.service

ipa group-add-member --groups=group_1 group_2
sss_cache -UG
sudo su -c "truncate -s0 /var/log/sssd/*.log"
getent group group_2

ipa group-remove-member --groups=group_1 group_2
sss_cache -UG
sudo su -c "truncate -s0 /var/log/sssd/*.log"
getent group group_2

# clean

ipa group-del group_2
ipa group_del group_1
ipa user-del adam
```
Configuration:
```bash
# cat /etc/sssd/sssd.conf

[domain/ipa.beta]
cache_credentials = True
krb5_store_password_if_offline = True
ipa_domain = beta
id_provider = ipa
auth_provider = ipa
access_provider = ipa
ipa_hostname = mirach.beta
chpass_provider = ipa
dyndns_update = True
ipa_server = _srv_, algol.beta
dyndns_iface = ens3
ldap_tls_cacert = /etc/ipa/ca.crt
entry_cache_timeout = 30
debug_level = 0xFFFF0

[sssd]
services = nss, sudo, pam, ssh
domains = ipa.beta
debug_level = 0xFFFFFF0

[nss]
homedir_substring = /home
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-248002682

[sssd PR#13][-Changes requested] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: -Changes requested

[sssd PR#13][synchronized] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Author: celestian
 Title: #13: MEMBEROF: Don't resolve members if they are removed
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/13/head:pr13
git checkout pr13

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

jhrozek commented:
"""
I think all review requests were addressed, the code now looks good to me and just reverting the non-code changes make the tests fail.

So I'm adding the Accepted label, just will wait a bit if others have some comments..
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-248556748

[sssd PR#13][+Accepted] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: +Accepted

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

lslebodn commented:
"""
On (21/09/16 02:18), Jakub Hrozek wrote:
> I think all review requests were addressed, the code now looks good to me and just reverting the non-code changes make the tests fail.
>
> So I'm adding the Accepted label, just will wait a bit if others have some comments..

Those changes are in generic LDAP code.
I plan to run some downstream test. I do not expect any failures
but I want to be sure :-)

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-248565407

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

lslebodn commented:
"""
Downstream related tests passed.
So +1

However, it seems that patch
"MEMBEROF: Don't resolve members if they are removed"
is not covered by integration test in 3rd patch.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-249677204

[sssd PR#13][-Accepted] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: -Accepted

[sssd PR#13][+Changes requested] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: +Changes requested

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
At first I would like to inform that ticket: https://fedorahosted.org/sssd/ticket/3186 wasn't bug. This patch is valid for ldap provider too. (I tested again manually with reproducer.)

In my opinion, the test remove_user_from_nested_group() is variation on reproducer provided in this PR. So it tests our case.

If I run intg. tests without this PR:
```
test_ldap.py::test_remove_user_from_group FAILED
test_ldap.py::test_remove_user_from_nested_group FAILED
```
If I run intg. tests with this PR: they passed.

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-251113687

[sssd PR#13][-Changes requested] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: -Changes requested

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

jhrozek commented:
"""
I think :@lslebodn is right, because I removed just the memberof patch. Now I have:
```
ab6dee1 TESTS: Adding intg. tests on nested groups
257b66a LDAP: Removing of member link from group
```
and make intgcheck happily passes.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-252289165

[sssd PR#13][+Changes requested] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: +Changes requested

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

jhrozek commented:
"""
So I wanted to write a test for the memberof patch, but I couldn't reproduce the ticket #2940 even without the memberof patch. I tried exactly the steps in the ticket and it seemed to me that just applying the ldap provider patch solves the issue.

@celestian can you give me steps to reproduce the bug that the patch is solving? Then I can write a test..
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253536311

[sssd PR#13][synchronized] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Author: celestian
 Title: #13: MEMBEROF: Don't resolve members if they are removed
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/13/head:pr13
git checkout pr13

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
So, I looked this patch set again. Both, ldap and memberof patch, can fix the issue itself. I removed memberof patch.

Original author of ldap patch is @sumit-bose -- do you agree with applying? I am sorry, I don't know how to write co-author to the patch.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253732056

[sssd PR#13][-Changes requested] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: -Changes requested

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

fidencio commented:
"""
On Fri, Oct 14, 2016 at 9:46 AM, celestian  wrote:
> So, I looked this patch set again. Both, ldap and memberof patch, can fix
> the issue itself. I removed memberof patch.
>
> Original author of ldap patch is @sumit-bose
> https://github.com/sumit-bose -- do you agree with applying? I am
> sorry, I don't know how to write co-author to the patch.

If you changed something in the patch, just add this* to the end of the
commit message.

*: Co-Author: Petr Čech

Best Regards,

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253732823

[sssd PR#13][synchronized] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Author: celestian
 Title: #13: MEMBEROF: Don't resolve members if they are removed
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/13/head:pr13
git checkout pr13

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
I changed the author of ldap patch to @sumit-bose.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253734041

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
Oh no, I tested it for ldap, not for ipa provider. Wait a minute a will test it again. :-(
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253735227

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
Great, the result is the same. Both patches could fix it. So we can use just ldap patch.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253736716

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

lslebodn commented:
"""
> Great, the result is the same. Both patches could fix it. So we can use just ldap patch.

What do you mean by both patches could fix it?

I aplied just a memboerof patch + test
```
git log --oneline -3
1cde694 TESTS: Adding intg. tests on nested groups
5a9c686 MEMBEROF: Don't resolve members if they are removed
761515e sssctl: call service with absolute path
```

and test failed
```
make -C src/tests/intg intgcheck-installed INTGCHECK_PYTEST_ARGS=" -k test_ldap.py"
//snip
test_ldap.py::test_user_2307bis_nested_groups PASSED
test_ldap.py::test_special_characters_in_names PASSED
test_ldap.py::test_extra_attribute_already_exists PASSED
test_ldap.py::test_add_user_to_group PASSED
test_ldap.py::test_remove_user_from_group FAILED
test_ldap.py::test_remove_user_from_nested_group FAILED

================================================ FAILURES ================================================
______________________________________ test_remove_user_from_group _______________________________________
Traceback (most recent call last):
  File "src/tests/intg/test_ldap.py", line 877, in test_remove_user_from_group
    ent.assert_group_by_name("group1", dict(mem=ent.contains_only()))
  File "src/tests/intg/ent.py", line 377, in assert_group_by_name
    assert not d, d
AssertionError: member list mismatch: 
unexpected members found:
['user1']
___________________________________ test_remove_user_from_nested_group ___________________________________
Traceback (most recent call last):
  File "src/tests/intg/test_ldap.py", line 953, in test_remove_user_from_nested_group
    dict(mem=ent.contains_only()))
  File "src/tests/intg/ent.py", line 377, in assert_group_by_name
    assert not d, d
AssertionError: member list mismatch: 
unexpected members found:
['user1']
================================ 102 tests deselected by '-ktest_ldap.py' ================================
========================== 2 failed, 15 passed, 102 deselected in 46.02 seconds ==========================
Makefile:728: recipe for target 'intgcheck-installed' failed
make: *** [intgcheck-installed] Error 1
make: Leaving directory 'intg/bld/src/tests/intg'
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253772653

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

lslebodn commented:
"""
> Great, the result is the same. Both patches could fix it. So we can use just ldap patch.

What do you mean by both patches could fix it?

I aplied just a memboerof patch + test
```
git log --oneline -3
1cde694 TESTS: Adding intg. tests on nested groups
5a9c686 MEMBEROF: Don't resolve members if they are removed
761515e sssctl: call service with absolute path
```

and test failed
```
make -C src/tests/intg intgcheck-installed INTGCHECK_PYTEST_ARGS=" -k test_ldap.py"
//snip
test_ldap.py::test_user_2307bis_nested_groups PASSED
test_ldap.py::test_special_characters_in_names PASSED
test_ldap.py::test_extra_attribute_already_exists PASSED
test_ldap.py::test_add_user_to_group PASSED
test_ldap.py::test_remove_user_from_group FAILED
test_ldap.py::test_remove_user_from_nested_group FAILED

================================================ FAILURES ================================================
______________________________________ test_remove_user_from_group _______________________________________
Traceback (most recent call last):
  File "src/tests/intg/test_ldap.py", line 877, in test_remove_user_from_group
    ent.assert_group_by_name("group1", dict(mem=ent.contains_only()))
  File "src/tests/intg/ent.py", line 377, in assert_group_by_name
    assert not d, d
AssertionError: member list mismatch: 
unexpected members found:
['user1']
___________________________________ test_remove_user_from_nested_group ___________________________________
Traceback (most recent call last):
  File "src/tests/intg/test_ldap.py", line 953, in test_remove_user_from_nested_group
    dict(mem=ent.contains_only()))
  File "src/tests/intg/ent.py", line 377, in assert_group_by_name
    assert not d, d
AssertionError: member list mismatch: 
unexpected members found:
['user1']
================================ 102 tests deselected by '-ktest_ldap.py' ================================
========================== 2 failed, 15 passed, 102 deselected in 46.02 seconds ==========================
Makefile:728: recipe for target 'intgcheck-installed' failed
make: *** [intgcheck-installed] Error 1
make: Leaving directory 'intg/bld/src/tests/intg'
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253772653

[sssd PR#13][+Accepted] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: +Accepted

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

jhrozek commented:
"""
btw since the memberof patch we couldn't test is gone, I'm fine with pushing these two.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253776046

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

lslebodn commented:
"""
On (14/10/16 04:33), Jakub Hrozek wrote:
> btw since the memberof patch we couldn't test is gone, I'm fine with pushing these two.

I would like to know Petr's explanation of his comment before pushing the
patches.

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253776910

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
I did manual testing with reproducer above. And I ran chmake (it is without intg., isn't it).
Now I check ldap patch with intg.
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253779213

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

lslebodn commented:
"""
On (14/10/16 04:48), celestian wrote:
> I did manual testing with reproducer above. And I ran chmake (it is without intg., isn't it).
> Now I check ldap patch with intg.

Then the question is why manual testing is different than newly added
integration tests.

BTW It is possible that patch in memberof plugin can safe some unnecessary
ldb operations and can be considered as perfomance enhancement.
But it's impossible to say that without proper integration test.

LS

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253780298

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[Lukas Slebodnik]

On (14/10/16 13:54), lslebodn wrote:
> URL: https://github.com/SSSD/sssd/pull/13
> Title: #13: MEMBEROF: Don't resolve members if they are removed
>
> lslebodn commented:
> """
> On (14/10/16 04:48), celestian wrote:
> > I did manual testing with reproducer above. And I ran chmake (it is without intg., isn't it).
> > Now I check ldap patch with intg.
>
> Then the question is why manual testing is different than newly added
> integration tests.
>
> BTW It is possible that patch in memberof plugin can safe some unnecessary
> ldb operations and can be considered as perfomance enhancement.
> But it's impossible to say that without proper integration test.

Bump,

I am still expecting answer to the comment even though that some
patches were pushed.

LS

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[Petr Cech]

On 10/17/2016 07:06 PM, Lukas Slebodnik wrote:
> On (14/10/16 13:54), lslebodn wrote:
> > URL: https://github.com/SSSD/sssd/pull/13
> > Title: #13: MEMBEROF: Don't resolve members if they are removed
> >
> > lslebodn commented:
> > """
> > On (14/10/16 04:48), celestian wrote:
> > > I did manual testing with reproducer above. And I ran chmake (it is without intg., isn't it).
> > > Now I check ldap patch with intg.
> >
> > Then the question is why manual testing is different than newly added
> > integration tests.
> >
> > BTW It is possible that patch in memberof plugin can safe some unnecessary
> > ldb operations and can be considered as perfomance enhancement.
> > But it's impossible to say that without proper integration test.
>
> Bump,
>
> I am still expecting answer to the comment even though that some
> patches were pushed.
>
> LS

Hi Lukas,

the intg. tests in points:
* Adding one user to the group
* let have group with two users -- removing users one by one
* let have user_1 in group_1; user_2 in group_2; group_1 and group_2 in 
group_3 -- removing groups (a and 2) one by one from group_3

Manual testing:

# prepare
ipa user-add --first=Adam --last=Adam --email=[e-mail] adam
ipa group-add group_1
ipa group-add-member --users=adam group_1
ipa group-add group_2

# reproducer

systemctl daemon-reload
sudo su -c "truncate -s0 /var/log/sssd/*.log"
sudo su -c "rm -f /var/lib/sss/db/*"
sudo su -c "rm -f /var/lib/sss/mc/*"
sudo systemctl restart sssd.service

ipa group-add-member --groups=group_1 group_2
sss_cache -UG
sudo su -c "truncate -s0 /var/log/sssd/*.log"
getent group group_2

ipa group-remove-member --groups=group_1 group_2
sss_cache -UG
sudo su -c "truncate -s0 /var/log/sssd/*.log"
getent group group_2

# clean

ipa group-del group_2
ipa group_del group_1
ipa user-del adam

In my opinion intg. tests and manual testing cover the same case.

I wrote 'memberof' patch when I have been working on similiar group 
tickets. I assume I confused ticket for `memberof` patch.

Regards

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[Lukas Slebodnik]

On (18/10/16 09:35), Petr Cech wrote:
> On 10/17/2016 07:06 PM, Lukas Slebodnik wrote:
> > On (14/10/16 13:54), lslebodn wrote:
> > > URL: https://github.com/SSSD/sssd/pull/13
> > > Title: #13: MEMBEROF: Don't resolve members if they are removed
> > >
> > > lslebodn commented:
> > > """
> > > On (14/10/16 04:48), celestian wrote:
> > > > I did manual testing with reproducer above. And I ran chmake (it is without intg., isn't it).
> > > > Now I check ldap patch with intg.
> > >
> > > Then the question is why manual testing is different than newly added
> > > integration tests.
> > >
> > > BTW It is possible that patch in memberof plugin can safe some unnecessary
> > > ldb operations and can be considered as perfomance enhancement.
> > > But it's impossible to say that without proper integration test.
> >
> > Bump,
> >
> > I am still expecting answer to the comment even though that some
> > patches were pushed.
> >
> > LS
>
> Hi Lukas,
>
> the intg. tests in points:
> Adding one user to the group
> let have group with two users -- removing users one by one
> let have user_1 in group_1; user_2 in group_2; group_1 and group_2 in
>
> group_3 -- removing groups (a and 2) one by one from group_3
>
> Manual testing:
>
> # prepare
> ipa user-add --first=Adam --last=Adam --email=[e-mail] adam
> ipa group-add group_1
> ipa group-add-member --users=adam group_1
> ipa group-add group_2
>
> # reproducer
>
> systemctl daemon-reload
> sudo su -c "truncate -s0 /var/log/sssd/*.log"
> sudo su -c "rm -f /var/lib/sss/db/*"
> sudo su -c "rm -f /var/lib/sss/mc/*"
> sudo systemctl restart sssd.service
>
> ipa group-add-member --groups=group_1 group_2
> sss_cache -UG
> sudo su -c "truncate -s0 /var/log/sssd/*.log"
> getent group group_2
>
> ipa group-remove-member --groups=group_1 group_2
> sss_cache -UG
> sudo su -c "truncate -s0 /var/log/sssd/*.log"
> getent group group_2
>
> # clean
>
> ipa group-del group_2
> ipa group_del group_1
> ipa user-del adam
>
> In my opinion intg. tests and manual testing cover the same case.
>
> I wrote 'memberof' patch when I have been working on similiar group tickets.
> I assume I confused ticket for `memberof` patch.

Do I understand it correctly that you will write a new integration test
for member of patch?

LS

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[Petr Cech]

On 10/19/2016 07:45 AM, Lukas Slebodnik wrote:
> On (18/10/16 09:35), Petr Cech wrote:
> > On 10/17/2016 07:06 PM, Lukas Slebodnik wrote:
> > > On (14/10/16 13:54), lslebodn wrote:
> > > > URL: https://github.com/SSSD/sssd/pull/13
> > > > Title: #13: MEMBEROF: Don't resolve members if they are removed
> > > >
> > > > lslebodn commented:
> > > > """
> > > > On (14/10/16 04:48), celestian wrote:
> > > > > I did manual testing with reproducer above. And I ran chmake (it is without intg., isn't it).
> > > > > Now I check ldap patch with intg.
> > > >
> > > > Then the question is why manual testing is different than newly added
> > > > integration tests.
> > > >
> > > > BTW It is possible that patch in memberof plugin can safe some unnecessary
> > > > ldb operations and can be considered as perfomance enhancement.
> > > > But it's impossible to say that without proper integration test.
> > >
> > > Bump,
> > >
> > > I am still expecting answer to the comment even though that some
> > > patches were pushed.
> > >
> > > LS
> >
> > Hi Lukas,
> >
> > the intg. tests in points:
> > Adding one user to the group
> > let have group with two users -- removing users one by one
> > let have user_1 in group_1; user_2 in group_2; group_1 and group_2 in
> >
> > group_3 -- removing groups (a and 2) one by one from group_3
> >
> > Manual testing:
> >
> > # prepare
> > ipa user-add --first=Adam --last=Adam --email=[e-mail] adam
> > ipa group-add group_1
> > ipa group-add-member --users=adam group_1
> > ipa group-add group_2
> >
> > # reproducer
> >
> > systemctl daemon-reload
> > sudo su -c "truncate -s0 /var/log/sssd/*.log"
> > sudo su -c "rm -f /var/lib/sss/db/*"
> > sudo su -c "rm -f /var/lib/sss/mc/*"
> > sudo systemctl restart sssd.service
> >
> > ipa group-add-member --groups=group_1 group_2
> > sss_cache -UG
> > sudo su -c "truncate -s0 /var/log/sssd/*.log"
> > getent group group_2
> >
> > ipa group-remove-member --groups=group_1 group_2
> > sss_cache -UG
> > sudo su -c "truncate -s0 /var/log/sssd/*.log"
> > getent group group_2
> >
> > # clean
> >
> > ipa group-del group_2
> > ipa group_del group_1
> > ipa user-del adam
> >
> > In my opinion intg. tests and manual testing cover the same case.
> >
> > I wrote 'memberof' patch when I have been working on similiar group tickets.
> > I assume I confused ticket for `memberof` patch.
>
> Do I understand it correctly that you will write a new integration test
> for member of patch?
>
> LS

Yes, I am writing test for memberof plugin right now :-)
The memberof patch is fix for ticket [1].

(So memberof patch has slightly different reproducer. I will add it to 
comment of the new ticket.)

[1] https://fedorahosted.org/sssd/ticket/3222

Regards

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

jhrozek commented:
"""
yes, if we don't know how to trigger the code, we shouldn't push it.

OK to push the two commits now?
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253781462

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

celestian commented:
"""
So, ldap patch passed intg. tests locally.

"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253782152

[sssd PR#13][-Accepted] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: -Accepted

[sssd PR#13][+Pushed] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

Label: +Pushed

[sssd PR#13][comment] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Title: #13: MEMBEROF: Don't resolve members if they are removed

jhrozek commented:
"""
master:
e0903f41922721edf292a9f7e6605a4519db53a1
eaf44bc07dda469a20be07d46737d93f518e2047
"""

See the full comment at https://github.com/SSSD/sssd/pull/13#issuecomment-253887578

[sssd PR#13][closed] MEMBEROF: Don't resolve members if they are removed

[fidencio]

URL: https://github.com/SSSD/sssd/pull/13
Author: celestian
 Title: #13: MEMBEROF: Don't resolve members if they are removed
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/13/head:pr13
git checkout pr13