On 22.08.2016 13:18, Oleg Fayans wrote:
ping for review

On 08/02/2016 01:11 PM, Oleg Fayans wrote:
Hi Martin,

I did! Thank you!

On 08/02/2016 12:31 PM, Martin Basti wrote:


On 01.08.2016 22:46, Oleg Fayans wrote:
The test was redesigned so that it actually tests against an AD user.
cleanly applies, passes lint and passes

https://paste.fedoraproject.org/399504/00843641/

Okay

Did you forget to send patches?

Martin^2


On 06/28/2016 01:40 PM, Oleg Fayans wrote:
Patch-0050 rebased against latest upstream branch

On 06/28/2016 10:45 AM, Oleg Fayans wrote:
Passing test output:

https://paste.fedoraproject.org/385774/71035231/













NACK for 0049.1

1)
PEP8: you must use 2 empty lines between functions

2)
+    new_args = " ".join(new_args + args)

you don't need this, run_command takes list as argument too
new_args.extend(args)

3)
To make it more usable you should add raiseonerr as kwarg to run_certutil (True as default)

NACK for 0050.2

1)
+        tasks.run_certutil(master, ['-L', '-n', cls.adcert1, '-a', '>',
+                                    cls.adcert1_file], cls.reqdir)
+        tasks.run_certutil(master, ['-L', '-n', cls.adcert2, '-a', '>',
+                                    cls.adcert2_file], cls.reqdir)

IMO thus should raise an error if failed, but previously you set raiseonerr=False (multiple times)

2)
+        cls.ad = cls.ad_domains[0].ads[0]
+        cls.ad_domain = cls.ad.domain.name
+        cls.aduser = "testuser@%s" % cls.ad_domain
+        cls.adcert1 = 'MyCert1'
+        cls.adcert2 = 'MyCert2'
+        cls.adcert1_file = cls.adcert1 + '.crt'
+        cls.adcert2_file = cls.adcert2 + '.crt'

New definitions of variables/constants should be directly in class not in install method, adding new class variables in classmethod is the same evil as adding instance variables outside __init__

3)
I have question, why do you need AD for this test? AFAIK you can use ID overrides without AD

Martin^3

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