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

[Martin Basti]

On 23/07/15 16:27, Martin Basti wrote:
> On 23/07/15 11:42, Oleg Fayans wrote:
>> Forgot to attach the new version, sorry!
>>
>> On 07/23/2015 10:32 AM, Oleg Fayans wrote:
>>> 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
>>>
>>
>>
>>
> Thank you ACK
>
> -- 
> Martin Basti
>
>
Pushed to master: e5acd01ed2971be779e788937493844a9926bb96

-- 
Martin Basti

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150727/c13d372c/attachment.htm>