On (28/08/14 18:37), Pavel Reichl wrote:
> On 08/28/2014 06:24 PM, Lukas Slebodnik wrote:
>> On (28/08/14 18:11), Pavel Reichl wrote:
>>> On 08/25/2014 02:05 PM, Jan Cholasta wrote:
>>>> Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
>>>>> On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
>>>>>> On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
>>>>>>> On 18.7.2014 06:41, William wrote:
>>>>>>>>>> Ignore that last patch, I messed up and
didn't include a .h file.
>>>>>>>>>> Here
>>>>>>>>>> is the fixed patch.
>>>>>>>>> This new patch seems to be based on some older
version of the
>>>>>>>>> patch, it
>>>>>>>>> uses wrong option name, sysdb_update_ssh_host still
has the confdb
>>>>>>>>> argument, etc.
>>>>>>>>>
>>>>>>>> Fixed, and fixed the documentation into the xml file,
removed the
>>>>>>>> references to the .pot file.
>>>>>>> I don't see any of the requested fixes in the patch.
>>>>>>>
>>>>>>> See attachment for fixed patch. I also did a few additional
tweaks to
>>>>>>> make
>>>>>>> the patch work when SSSD is updated from a previous version
and when
>>>>>>> building without SSH bits.
>>>>>> Awesome thank you, I guess some of us need to review the patch
now
>>>>>> you're also an author.
>>>>> While Jan was so kind to give me some hints on how to test patch
>>>>> properly, we found out a little bug. So Jan is going to prepare an
>>>>> updated patch.
>>>>>
>>>>> I'll do the review then.
>>>> I can't reproduce the issue, maybe something else was wrong in your
setup?
>>>>
>>>> For the record, I installed IPA in a VM and then ran these commands on
it:
>>>>
>>>> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
>>>> [no errors]
>>>> $ cat /var/lib/sss/pubconf/known_hosts
>>>> [not empty]
>>>> # systemctl stop ipa
>>>> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
>>>> [no errors]
>>>> $ cat /var/lib/sss/pubconf/known_hosts
>>>> [not empty]
>>>> # sss_cache -H
>>>> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
>>>> [no errors]
>>>> $ cat /var/lib/sss/pubconf/known_hosts
>>>> [empty]
>>>> # systemctl start ipa
>>>> $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME
>>>> [no errors]
>>>> $ cat /var/lib/sss/pubconf/known_hosts
>>>> [not empty]
>>>>
>>>> Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I
>>>> have it non-empty as expected.
>>>>
>>>> (sorry for the delay, I was on leave)
>>>>
>>> This test ran OK this time, so I think you are right.
>>>
>>> I have some comments to the patch itself.
>>>
>>> I'm not personally fan of big patches like this one. Could you at least
>>> separate the man page changes to separate patch - so we can ask some native
>>> speaker to review it.
>> I am not big fan of splitting patches just for "man page changes"
>>
>> LS
> I like small patches, each doing one think and one think only. But I know I
> can't get always what I want.
>
> Still I believe that it would be a good policy to have man page updates in
> separate patch as we already have policy to ask native speaker to do review
> such changes. I think, it's a way more convenient for reviewer not to have
> search through the patch for man page changes and as a bonus he gets
> reviewed-by achievement. :-)
>
> What are you reasons not to have separate patch?
>
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/021296.html
LS
I have read that already no need to link messages a few minutes old.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel