[Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name

[Lukas Slebodnik]

On (22/04/16 08:09), Martin Basti wrote:
>
>
>On 22.04.2016 05:02, Rob Crittenden wrote:
>> Lukas Slebodnik wrote:
>> > On (21/04/16 16:42), Rob Crittenden wrote:
>> > > Lukas Slebodnik wrote:
>> > > > On (21/04/16 19:25), Petr Vobornik wrote:
>> > > > > related but does not implement
>> > > > > https://fedorahosted.org/freeipa/ticket/5806
>> > > > > -- 
>> > > > > Petr Vobornik
>> > > > 
>> > > > >  From b9b8716ec9ba5a5cdbed1f6cdedf7cff8878f08f Mon Sep 17
>> > > > > 00:00:00 2001
>> > > > > From: Petr Vobornik <pvoborni at redhat.com>
>> > > > > Date: Thu, 21 Apr 2016 19:23:31 +0200
>> > > > > Subject: [PATCH] ipa-client-install: fix typo in nslcd service name
>> > > > > 
>> > > > > related but does not implement
>> > > > > https://fedorahosted.org/freeipa/ticket/5806
>> > > > > ---
>> > > > > client/ipa-client-install | 2 +-
>> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/client/ipa-client-install b/client/ipa-client-install
>> > > > > index c38843f85639a9118cd1a471709992690643d79a..0e6e65c4ad4ce01fe1257eee4bb2633a70c3de4e
>> > > > > 100755
>> > > > > --- a/client/ipa-client-install
>> > > > > +++ b/client/ipa-client-install
>> > > > > @@ -2938,7 +2938,7 @@ def install(options, env, fstore, statestore):
>> > > > >                   nscd.service_name)
>> > > > > 
>> > > > >       nslcd = services.knownservices.nslcd
>> > > > > -    if nscd.is_installed():
>> > > > > +    if nslcd.is_installed():
>> > > > >           save_state(nslcd)
>> > > > 
>> > > > I thought that milestone "Future Releases" has lower priority
>> > > > then "FreeIPA 4.4 Backlog"
>> > > > 
>> > > > Therefore I would prefer to close ticket #5806 and implement
>> > > > following one
>> > > > https://fedorahosted.org/freeipa/ticket/5557#comment:2
>> > > 
>> > > I don't understand what you are suggesting. Tickets aren't
>> > > swapped like this
>> > > and certainly non-related bugs aren't closed for another.
>> > > 
>> > > This patch just fixes an obvious one-liner as agreed upon in
>> > > triage. The rest
>> > > will be potentially addressed later.
>> > > 
>> > > ACK on the patch.
>+1
>> > > 
>> > I cannot see a reason to fix oneliner.
>> > This code is not tested and should be removed (#5557)
>> 
>> I fail to see the controversy here. These are two completely and
>> absolutely unrelated bugs.
>> 
>> > Or someone should write integration test.
>> > 
>> > I'm sorry but conditional-NACK (missing integration test)
>> 
>> I've been out of the project a while, so perhaps my knowledge is stale,
>> but AFAIU upstream QE writes the integration tests, not the developers.
>> This was at least true when I was a daily contributor.
>>
You were used to old style development model (waterfall).
And management mentioned that we should be more agile.
All agile methodologies assume that also developers write tests.

>> At least at one time QE did testing using --no-sssd. Whether that is
It should have been a long time ago.
Modern freeipa client (4.4) requires sssd and many features
will not work with nslcd. Trust, views ...

>> still the case I don't know but I do remember fixing bugs around it. And
>> this particular bug might not be immediately apparent unless a specific
>> configuration was present. I noticed the bug while reading code, not in
>> production.
>>
>> rob
>> 
>Patch do exactly the same as was agreed on devel meeting. I don't see reason
>why we shouldn't fix oneliner.
>
Because it's not tested and there might be much more things which
does not work. It should be either tested or removed.

Ticket #5557 was triaged before #5806. (4 monts ago vs 10 days ago).
The purpose of ticket "#5557" was to have a single line change in
freeipa spec file. But after a triage it was decided that this part of code
will be deprecated and later removed.

I know that this patch was a oneliner but I do not see a reason why
freeipa developers should waste time with fixing unested code.
It's obvious it could not work since 2013-11-07 and nonody has complained yet.
It's another reason to remove this code.

Another reason is that freeipa-client does not have a dependency
on nss-pam-ldapd.

BTW I moved ticket #5557 to reconsider it one more time.

LS