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

[Oleg Fayans]

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
>
>
>
>

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0001.2-test-topology-plugin-is-listed-among-DS-plugins.patch
Type: text/x-patch
Size: 4274 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150722/66c7de59/attachment.bin>