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
