On 06.09.2016 13:57, 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



Did you forget to send patches?


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:


NACK for 0049.1

PEP8: you must use 2 empty lines between functions


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

you don't need this, run_command takes list as argument too

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

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


NACK for 0050.2

+ 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

+        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

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.


I still don't see the reason why AD trust is needed. Default trust ID view is added just by ipa-adtrust-install, adding trust is not needed for current implementation. You don't need AD for this, IDviews is generic feature not just for AD. Is that user configured on AD side?

The test itself looks for me as just API/CLI test. IMO it can be in ipatests/test_xmlrpc/test_idviews_plugin.py or ipatests/test_xmlrpc/test_add_remove_cert_cmd.py

I don't see any integration with SSSD in that test, just pure IPA CLI test, shouldn't be this tested against SSSD here?

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

Reply via email to