On 07/20/2016 04:11 PM, Lenka Doudova wrote:



On 07/20/2016 02:04 PM, Martin Babinsky wrote:
On 07/15/2016 06:10 PM, Lenka Doudova wrote:
Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka




Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems
ok.

However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something
more specific like:

"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78
character width (IIRC) but your lines are way too long.

A nice way to automate this in vim is to highlight the text, enter
command mode and run 'gq', that should format the text for you.

3.) You did not add upstream ticket URL to the end of the message,
please do so.

There is also an extraneous whitespace here:
"""
             else:
                 if type(value) is list:
                     self.attrs[key] = value
                 else:
                     self.attrs[key] = [value]
+
         for key, value in expected_updates.items():
             if value is None or value is '' or value is u'':
                 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format

Hi,

thanks for review, fixed patch attached.

Lenka

Thanks, ACK.

Pushed to master: 9093647f867e49bffc9042185b1765b48fe7400a

--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to