[Freeipa-devel] [PATCHES 523-525] replica install: add remote connection check over API

[Tomas Babej]

On 12/11/2015 03:40 PM, Jan Cholasta wrote:
> On 11.12.2015 08:03, Jan Cholasta wrote:
>> On 11.12.2015 07:08, Jan Cholasta wrote:
>>> On 10.12.2015 15:56, Martin Babinsky wrote:
>>>> On 12/10/2015 09:48 AM, Jan Cholasta wrote:
>>>>> On 9.12.2015 16:38, Jan Cholasta wrote:
>>>>>> On 9.12.2015 14:52, Jan Cholasta wrote:
>>>>>>> On 9.12.2015 10:02, Jan Cholasta wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> the attached patches fix
>>>>>>>> <https://fedorahosted.org/freeipa/ticket/5497>.
>>>>>>>
>>>>>>> Note that this needs selinux-policy fix to work, so put SELinux into
>>>>>>> permissive mode for testing:
>>>>>>> <https://bugzilla.redhat.com/show_bug.cgi?id=1289930>.
>>>>>>
>>>>>> Updated patches attached.
>>>>>
>>>>> I screwed up a change in patch 524 and accidentally included a
>>>>> chunk of
>>>>> code in patch 525 that doesn't belong in it.
>>>>>
>>>>> Updated patches attached.
>>>>>
>>>>>
>>>>>
>>>>
>>>> Patches work as expected and I was not able to find any functional
>>>> problem.
>>>>
>>>> I have a question about the naming of the oddjob helper script: the one
>>>> related to trusts is named 'com.redhat.idm.trust-fetch-domains', and
>>>> the
>>>> conncheck runner is named 'org.freeipa.server.conncheck'. I don't want
>>>> to start another bikeshedding conversation but shouldn't we named them
>>>> in a consistent fashion (either rename the first one in separate patch
>>>> or rename the new helper to com.redhat.idm.server.conncheck)?
>>>>
>>>> I understand that as an upstream, we should go with the 'org.freeipa.*'
>>>> convention, but having two helpers with different prefixes makes me
>>>> sad.
>>>
>>> If you look at the larger picture, org.freeipa is the consistent name.
>>> It makes me sad as well, but mistakes should be corrected. This is
>>> similar to how we use PEP8 in new code, but do not fix it in old code
>>> just for the sake of fixing it.
>>>
>>>>
>>>> That is a nitpick though, it does not affect the overall functionality
>>>> of the patches so ACK.
>>>
>>> Thanks for the review. The current patch 523 breaks the trusts oddjob
>>> with SELinux in enforcing mode, I will send an update which corrects
>>> that, until bug 1289930 is fixed.
>>
>> Updated patches attached.
> 
> Rebased on top of current master.
> 
> 
> 

ACK from me too,
Pushed to master: 14a44ea47bf9a617019ebc91fbe272215c428d82