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?
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel