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