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