On (28/08/14 19:19), Pavel Reichl wrote:
On 08/28/2014 07:13 PM, Lukas Slebodnik wrote:
>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.
In this case, I
don't understand why did you ask?
(no reply needed :-)
LS