[Freeipa-devel] [PATCH 0001] Test Topology plugin is listed among DS plugins
Martin Basti
mbasti at redhat.com
Thu Jul 23 14:27:19 UTC 2015
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150723/c15475f4/attachment.htm>
More information about the Freeipa-devel
mailing list