Re: [Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name
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 >> > > > > 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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name
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 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. At least at one time QE did testing using --no-sssd. Whether that is 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. Pushed to master: a023dcbc5c2e60d29938588845905b62986c3a93 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name
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 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. 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. At least at one time QE did testing using --no-sssd. Whether that is 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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name
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 >> > 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. > I cannot see a reason to fix oneliner. This code is not tested and should be removed (#5557) Or someone should write integration test. I'm sorry but conditional-NACK (missing integration test) LS -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name
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 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. rob -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] 957 ipa-client-install: fix typo in nslcd service name
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 >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 LS -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
