[Freeipa-devel] [PATCH 0001] Test Topology plugin is listed among DS plugins

[Oleg Fayans]

Hi Martin,

On 07/22/2015 05:48 PM, Martin Basti wrote:
> On 22/07/15 15:19, Oleg Fayans wrote:
>> Hi Martin,
>>
>> Fixed.
>>
>> On 07/22/2015 09:26 AM, Martin Basti wrote:
>>> On 22/07/15 09:23, Oleg Fayans wrote:
>>>> Hi Martin,
>>>>
>>>> Patch updated. Thank you for the review!
>>>>
>>>> On 07/21/2015 05:45 PM, Martin Basti wrote:
>>>>> On 20/07/15 14:07, Oleg Fayans wrote:
>>>>>> Hi Martin,
>>>>>>
>>>>>> Updated.
>>>>>>
>>>>>>
>>>>>> On 07/20/2015 12:46 PM, Martin Basti wrote:
>>>>>>> On 20/07/15 11:57, Oleg Fayans wrote:
>>>>>>>> +        pwfile = api.env.dot_ipa + os.sep + ".dmpw"
>>>>>>>> +        if ipautil.file_exists(pwfile):
>>>>>>>> +            fp = open(pwfile, "r")
>>>>>>>> +            dm_password = fp.read().rstrip()
>>>>>>>> +            fp.close()
>>>>>>>> +        else:
>>>>>>> Hello,
>>>>>>>
>>>>>>> 1) Can you use os.path.join() instead of "+ os.sep +" please
>>>>>>>
>>>>>>> 2) Can you use with statement with file?
>>>>>>>
>>>>>>> with open(pwfile, "r") as f:
>>>>>>>     dm_password = f.read().rstrip()
>>>>>>>
>>>>>>> 3) Please keep PEP8 in new code
>>>>>>>
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:30:80: E501 
>>>>>>> line too long (102 > 79 characters)
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:33:80: E501 
>>>>>>> line too long (92 > 79 characters)
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:39:80: E501 
>>>>>>> line too long (124 > 79 characters)
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:44:80: E501 
>>>>>>> line too long (92 > 79 characters)
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:45:48: E128 
>>>>>>> continuation line under-indented for visual indent
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:45:80: E501 
>>>>>>> line too long (89 > 79 characters)
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:46:48: E128 
>>>>>>> continuation line under-indented for visual indent
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:46:80: E501 
>>>>>>> line too long (89 > 79 characters)
>>>>>>> ./ipatests/test_ipaserver/test_topology_plugin.py:58:80: E501 
>>>>>>> line too long (87 > 79 characters)
>>>>>>>
>>>>>>> 4) Missing nose import
>>>>>>>             raise nose.SkipTest("No directory manager password 
>>>>>>> in %s" % pwfile)
>>>>>>>
>>>>>>> 5) Can you use sets here instead of sorted lists?
>>>>>>> assert(sorted(entry.keys()) == sorted(pluginattrs.keys()))
>>>>>>>
>>>>>>>
>>>>>>> Martin^2
>>>>>>>
>>>>>>
>>>>> 1)
>>>>> Sorry, I didn't notice before, but there is missing header in that 
>>>>> file.
>>>>>
>>>>> 2)
>>>>> You don't need to specify ldap_uri, you just need to call 
>>>>> ldap2(api), by default api.env.ldap_uri is used, which is the same 
>>>>> as you specified
>>>>>
>>>>> 3)
>>>>> Can you indent values of dict which are on newline? It is readable 
>>>>> better.
>>>>>             u'nsslapd-topo-plugin-shared-config-base':
>>>>>                 [u'cn=ipa,cn=etc,dc=example,dc=com'],
>>>>>             u'nsslapd-pluginDescription': [u'ipa-topology-plugin'],
>>>>>
>>>>> 4)
>>>>> Please use lower F as variable, in python we use capital letters 
>>>>> for class definitions
>>>>>             with open(pwfile, "r") as F:
>>>>>                 dm_password = F.read().rstrip()
>>>>>
>>>>> Otherwise it works as expected.
>>>>>
>>>>> Martin^2
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>> Sorry.
>>> You added there old license format, we now use in new files new format
>>>
>>> #
>>> # Copyright (C) 2015  FreeIPA Contributors see COPYING for license
>>> #
>>>
>>
> I cannot apply the last patch
>
> $ git am 
> freeipa-ofayans-0001.3-test-topologyplugin-is-listed-among-DS-plugins.patch 
> -3
> Applying: Added test - topology plugin is listed among DS plugins
> fatal: corrupt patch at line 83
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
>
Fixed. Tested it locally, it applies

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.