[Freeipa-devel] [PATCH 0029][Tests] Adding authentication test to trust test suite

[Martin Babinsky]

On 07/22/2016 11:20 AM, Lenka Doudova wrote:
>
>
> On 07/20/2016 02:28 PM, Martin Babinsky wrote:
>> On 07/19/2016 10:41 AM, Lenka Doudova wrote:
>>> Hi,
>>>
>>>
>>> this patch adds authentication test (specifically "kinit -E
>>> ipauser at IPADOMAIN") to basic trust test suite, as requested by Sumit.
>>>
>>> Intended to be applied after my patches 25.4 and 26.3 (already waiting
>>> to be pushed).
>>>
>>>
>>> Lenka
>>>
>>>
>>>
>>
>> Hi Lenka,
>>
>> Code review:
>>
>> 1.) You have several PEP8 transgressions in the patch, please fix them:
>> """
>> $ git show -U0 | pep8 --diff
>> ./ipatests/test_integration/test_trust.py:172:34: E127 continuation
>> line over-indented for visual indent
>> ./ipatests/test_integration/test_trust.py:176:35: E128 continuation
>> line under-indented for visual indent
>> ./ipatests/test_integration/test_trust.py:180:27: E231 missing
>> whitespace after ','
>> """
>>
>> 2.)
>> +
>> +
>> +def unlock_principal_password(user, oldpw, newpw, master):
>> +    container_user = "cn=users,cn=accounts"
>> +    basedn = master.domain.basedn
>> +
>> +    userdn = "uid={},{},{}".format(user, container_user, basedn)
>> +
>> +    args = [paths.LDAPPASSWD, '-D', userdn, '-w', oldpw, '-a', oldpw,
>> +            '-s', newpw, '-x']
>> +    return run(args)
>>
>> there is already a function with the same name in other module:
>>
>> """
>> git grep -ni 'def unlock_principal_password' ipatests
>> ipatests/test_integration/util.py:82:def
>> unlock_principal_password(user, oldpw, newpw, master):
>> ipatests/util.py:676:def unlock_principal_password(user, oldpw, newpw):
>> """
>>
>> Having functions with the same names in different modules makes for
>> poor coding practice IMHO. Please rename the function to something
>> like "ldappasswd_user" or something like that so that we have a
>> distinction.
>>
>> Also, you should call ldappasswd directly on master (since you pass it
>> as an argument anyway) using "master.run_command(args)". You should
>> *not* run any in-test code on the controller unless absolutely necessary.
>>
>> You can then remove the ipautil.run import from the beginning of the
>> module.
>>
>> Commit message woes:
>>
>> 1.) vague summary is vague, I would rather see something like:
>>
>> """
>> test that IPA user can kinit using enterprise principal with IPA domain
>> """
>>
>> 2.) Commit message body is longer than 78 characters.
>>
>> 3.) there is no ticket URL, I think you can link
>> https://fedorahosted.org/freeipa/ticket/6036 or create a new ticket
>> for the change.
>>
> Hi,
>
> thanks for review, fixed (and renamed) patch attached.
>
> Lenka

Thanks, ACK.

Pushed to master: 648b5afa2f2d01d99c1cf2d1f4a87a5da4461246

-- 
Martin^3 Babinsky