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?
>
>>+ </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!
Honza,
we have a template in git repo ".git-commit-template"
For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc
LS