On (02/08/16 16:39), Michal Židek wrote: >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?= <[email protected]> >> > > > 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. > Let me use a different example. There is a crash[1] in current master if group or use contains brackets. There were two "sub-bugs" in code. One in responder and other in back-end. It does not make a sense to fix just one part. Because from user point of view it still does not work. It only make a sense to split fixes into two patches; but that's the developer point of view.
>It does not have high priority (now) and I am OK >if the two tickets are solved together later. > That is not true. #2275 should have been fixed in 1.13.1 but it does not work. It is not important whether it's a regression or it was fixed just a partially in #2275. It has to be fixed very soon because there is a related downstream ticket BZ1118257 and we do not have a lot of time. So it has the highest priority. BTW the best way how to fix a bugs is to firstly write a test. manual reproducer is just a wasting of time. I did it in [1] :-) LS [1] https://lists.fedorahosted.org/archives/list/[email protected]/thread/VTU7U2ST7IBSIIY6JCQVCBSAQDXXUCBB/ _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
