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?= <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.
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/sssd-devel@lists.fedorahoste...