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

[Martin Basti]

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
#

-- 
Martin Basti