Hi Oleg,
thank you, now it's completely different game.
Please add prefix to commit message summaries. Simply prepending "tests:
" should be OK.
0041 - -h is deprecated in favor of -H.
0062 - 0068 - LGTM
0069 - I see 2 unrelated changes in the patch, please split them:
- 1 - certutil - > paths.CERTUTIL
- 2 - assert
0070 - I see 2 unrelated changes in the patch, please split them:
- 1 - teardown
- 2 - TestReplicaInstall.setUp -> TestReplicaInstall.install
0071 - typos in commit message, I see 5 unrelated changes in that patch:
- 1 - error messages in assert
- 2 - certificates used
- 3 - verify_installation called only in DOMAIN_LEVEL_0.
- 4 - TestCertinstall.install
- 5 - TestCertinstall.certinstall
0072 - 0077 - LGTM
On 09/09/16 15:22, Oleg Fayans wrote:
Hi David, team
According to your suggestions I've splitted my commits so that each
commit addresses some particular problem. One patch (0071) still
contains several unrelated fixes, but they mostly reflect changes in
error messages and really small but numerous bugfixes that I did not
consider worthy of a separate commit each. Please, whenever you have a
free time take a look at this new bunch of patches.
Thanks!
On 09/06/2016 04:41 PM, David Kupka wrote:
Hi Oleg!
0013 - It looks like there are two unrelated changes, addition of CRL
distribution extension and creating certificate signed by no longer
existing CA. Please create separate patch for each of the changes, and
describe the change and reason for it in commit messages.
0014 - Could you please split the patch to "numerous" commit each fixing
one error? Please also describe each fix so everyone has at least vague
idea about the patch without reading its code. Also why do you introduce
global variable config, I don't see its used anywhere.
0039 - It looks like multiple different changes and commit message says
nothing again. Please split and describe what did you change and why.
0041 - Looks like weird workaround to me. It would be better to
investigate the root cause and fix it. Or at least describe the cause in
commit message and code comment if it can't be fixed. Also "-h is
deprecated in favor of -H" says man 1 ldapmodify.
On 05/09/16 14:32, Oleg Fayans wrote:
Hi guys,
Finally the ca-less tests are stable. Here in the attachment is the full
set of necessary patches.
On 08/09/2016 10:57 AM, Oleg Fayans wrote:
Hi all,
Bump for the review of the 0013 patch. The script it addresses can be
reused in some WebUI tests - one more reason to have it reviewed/merged
The rest patches should be re-tested, since they were prepared a good
while ago
On 05/10/2016 05:08 PM, Oleg Fayans wrote:
Hi David,
After quite a while and some more struggles here comes the updated
version of the patch together with other patches fixing things in
ipatests/test_integration/tasks.py
Server and replica installation was refactored in a way to utilize the
code from tasks.py as much as it is possible
The full set of necessary patches is attached
On 04/20/2016 10:35 AM, David Kupka wrote:
On 19/04/16 11:13, Oleg Fayans wrote:
OK, that one, though passing lint, did not actually work. I gave
up my
attempts to define method decorators inside the class. Now it passes
lint AND works:)
Hi Oleg!
1) Current commit message is useless. Please use it to describe
what is
the point of the patch.
2) $ git show -U0 | pep8 --diff
./ipatests/test_integration/test_caless.py:66:1: E302 expected 2
blank
lines, found 1
./ipatests/test_integration/test_caless.py:74:1: E302 expected 2
blank
lines, found 1
./ipatests/test_integration/test_caless.py:820:5: E303 too many blank
lines (2)
./ipatests/test_integration/test_caless.py:825:80: E501 line too long
(80 > 79 characters)
./ipatests/test_integration/test_caless.py:1035:44: E225 missing
whitespace around operator
3) Isn't there a way to do this with pytest's fixtures?
+def server_install_teardown(func):
+ def wrapped(*args):
+ try:
+ func(*args)
+ finally:
+ args[0].uninstall_server()
+ return wrapped
+
+def replica_install_teardown(func):
+ def wrapped(*args):
+ try:
+ func(*args)
+ finally:
+ # Uninstall replica
+ replica = args[0].replicas[0]
+ tasks.kinit_admin(args[0].master)
+ args[0].uninstall_server(replica)
+ args[0].master.run_command(['ipa-replica-manage',
'del',
+ replica.hostname,
'--force'],
+ raiseonerr=False)
+ args[0].master.run_command(['ipa', 'host-del',
+ replica.hostname],
+ raiseonerr=False)
+ return wrapped
+
There is a standard pytest method called 'method_teardown', that is
indent to be executed after each test method, but with our setup it
does
not work.
4) Is it necessary to create the $TEST_DIR in the test? Isn't it
created
by the framework?
+ host.transport.mkdir_recursive(host.config.test_dir)
Removed.
5) I don't think the comment match the code.
+ # Remove CA cert in /etc/pki/nssdb, in case of failed
(un)install
+ for host in cls.get_all_hosts():
+ cls.uninstall_server(host)
+
super(CALessBase, cls).uninstall(mh)
Not actual anymore
6) No! Create list with one element, iterate that list and append
every
item to the other list. Maybe there's better way (Hint: append).
I've seen this on multiple places.
if unattended:
args.extend(['-U'])
Agreed
7) Why don't you (extend and) use
ipatests.test_integaration.tasks.(un)install_{master,replica}?
This could be done pretty much all over the code.
host.run_command(['ipa-server-install', '--uninstall',
'-U'])
8) Use ipaplatform.paths for certutil and other binaries. If the
binary
is not there feel free to add it.
I've seen this on multiple places.
+ host.run_command(['certutil', '-d', paths.NSS_DB_DIR, '-D',
+ '-n', 'External CA cert'],
+ raiseonerr=False)
+ # A workaround
forhttps://fedorahosted.org/freeipa/ticket/4639
+ result = host.run_command(['certutil', '-L', '-d',
+ paths.HTTPD_ALIAS_DIR])
+ for rawcert in result.stdout_text.split('\n')[4: -1]:
+ cert = rawcert.split(' ')[0]
+ host.run_command(['certutil', '-D', '-d',
paths.HTTPD_ALIAS_DIR,
+ '-n', cert])
Done
9) certmonger is system service. You can check if is is .enabled()
and
.running(). And IIUC the comment is negation of what the code does.
# Verify certmonger was not started
result = host.run_command(['getcert', 'list'],
raiseonerr=False)
- assert result > 0
- assert ('Please verify that the certmonger service has
been '
- 'started.' in result.stdout_text),
result.stdout_text
+ assert result.returncode == 0
10) What is the point of calling uninstall_server() when it will be
called in the finally block of server_install_teardown anyway?
+ @server_install_teardown
def test_revoked_http(self):
"IPA server install with revoked HTTP certificate"
if result.returncode == 0:
+ self.uninstall_server()
raise nose.SkipTest(
"Known CA-less installation defect, see "
+"https://fedorahosted.org/freeipa/ticket/4270";)
assert result.returncode > 0
Removed
Nitpick) Do not mix fixing typos/grammar/spelling/style with
functional
changes.
- def test_incorect_http_pin(self):
+ @pytest.mark.xfail(reason='freeipa ticket 5378')
+ def test_incorrect_http_pin(self):
"Install new HTTP certificate with incorrect PKCS#12
password"
Removed
--
David Kupka
--
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
