Dne 2.9.2014 v 16:37 Lukas Slebodnik napsal(a):
On (02/09/14 16:21), Pavel Reichl wrote:
>
> On 09/01/2014 02:27 PM, Pavel Reichl wrote:
>>
>> On 08/29/2014 10:41 AM, Jan Cholasta wrote:
>>> Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
>>>>
>>>> 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.
>>>>
>>>> Also new options in sss_cache -h and -H are not documented in man
>>>> pages,
>>>> could you document them?
>>>>
>>>> Please see some minor comments inline:
>>>
>>> Fixed everything, except:
>>>
>>>>> + ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs,
mod_op);
>>>>
>>>> We should check return value here, right?
>>>
>>> We want to return this value directly, there's no need to check it
(other
>>> sysdb_set_*_attr functions do this as well).
>>>
>>> Updated patch attached.
>>>
>> Thanks,
>>> + <varlistentry condition="with_ssh">
>>> + <term>
>>> + <option>-h</option>,<option>--ssh-host</option>
>>> + <replaceable>autofs-map</replaceable>
>> ^^^^^
>>
>> copy&paste mistake?
Fixed.
>>
>>> + </term>
>>> + <listitem>
>>> + <para>
>>> + Invalidate SSH public keys of a specific host.
>>> + </para>
>>> + </listitem>
>>> + </varlistentry>
>>
> Could you also add ticket number (#2358) to the commit message? Thanks!
I wasn't aware there is a ticket for this.
Honza,
we have a template in git repo ".git-commit-template"
For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc
OK, thanks.
LS
Updated patch attached.
--
Jan Cholasta