On 08/02/2016 03:46 PM, Lukas Slebodnik wrote:
On (02/08/16 13:50), Petr Cech wrote:
> On 08/02/2016 12:41 PM, Petr Cech wrote:
>> On 08/02/2016 11:09 AM, Michal Židek wrote:
>>> Hi!
>>>
>>> When reviewing Petr's netgroup patch I found some issues
>>> with netgroups when using IPA provider.
>>>
>>> Attached patch fixes one of them.
>>>
>>> I filed ticket for the other issue here:
>>>
https://fedorahosted.org/sssd/ticket/3117
>>>
>>> Reviewing this is not priority for this week, but
>>> I already had the patch and wanted to put the
>>> list.
>>>
>>> Thanks,
>>> Michal
>>>
>>> 0001-ipa_netgroups-Lowercase-key-to-htable.patch
>>>
>>>
>>> From 2e5022452d7002e44ad17a59b4d5b12958721997 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
>>> Date: Thu, 28 Jul 2016 12:55:46 +0200
>>> Subject: [PATCH] ipa_netgroups: Lowercase key to htable
>>>
>>> Fixes:
>>>
https://fedorahosted.org/sssd/ticket/3116
>>>
>>> We lowercase the search key when storing
>>> entries into the hash table, but do not
>>> do it when we search for them. As a result
>>> we were not able to find netgroup by
>>> DN.
>>> ---
>>
>> Hi Michal,
>>
>> it looks good to me (aka LGTM).
>> I am waiting for CI.
>
> CI passed:
>
http://sssd-ci.duckdns.org/logs/job/50/67/summary.html
>
> ACK
>
> And for test... I think you might file a ticket.
>
I'm sorry I have a different opinion.
Michal tried to resolve nested netgroup membership
which should have been fixed in #2275
This patch is just a fix to the partial problem.
a) it does not fix a bug
It does fix one.
b) it does not have an integration test
c) id does not have an unit test
^
Yes we do not have upstream tests for netgroups.
We should implement them.
Many people consider a code without code coverage as a legacy code.
We need to avoid such situations and moreover we need to avoid possible
regressions in future. Therefore nested netgroup membership in IPA
need to be fixed (and not just a partially). And we should have a test
for this bug. I don't might which king of test. But IMHO the simplest would
be to write an integration test to freeipa project.
=> Conditional NACK untill resolvimng previously mentioned issues.
LS
The purpose of this patch was to fix one
particular bug in code that I found when looking into
the netgroups. It fixes one issue with netgroups,
not all of them. I sent it to list and filed the
tickets so that I do not need to keep the information
about it in my head.
It does not have high priority (now) and I am OK
if the two tickets are solved together later.
Michal