Ping for review.

On 09/06/2016 01:57 PM, Oleg Fayans wrote:
The test is updated to clean up after itself

On 09/06/2016 12:57 PM, Oleg Fayans wrote:
Hi Martin,

Thanks for the review. The updated patches are attached. Please, see my
comments below

On 08/30/2016 01:58 PM, Martin Basti wrote:


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

Fixed


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

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

The list-based approach does not work with shell redirects which are
heavily used in the certs_id_idoverrides test. Thus, this trick is
really needed


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

Done


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)

Agreed. Done


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__

Fair point. Fixed


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

Correct. You can, but the workflow would be slightly different. For
example, you can not issue and sign cert requests for AD-users the way
you would do it for local users. We want to have tests that can be taken
by end-users as example how to use our software, that's why it is better
to be as close to real-world use-cases as it is possible.


Martin^3








--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

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