On 09/03/2014 02:40 PM, Jan Cholasta wrote:
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.
Neither did I till yesterday's
evening.
>
> Honza,
> we have a template in git repo ".git-commit-template"
>
> For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc
OK, thanks.
>
> LS
Updated patch attached.
Thanks,
ACK
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel