On 12/15/2015 10:29 AM, Oleg Fayans wrote:
The updated patches are attached. Patch 0017 includes all changes from
patch 0018, so, if you approve this one, there would be no need to
continue with the review of 0018. This one contains all changes related
to you remarks from 0018 review. Please see my explanation on the
stdout+stderr part in the thread from patch 0018.
With these two patches applied one of the tests fails due this bug:
That may be due to SELinux preventing oddjob to run
'org.freeipa.server.conn_check'. Try running the tests with permissive
SELinux on all VMs.
On 12/09/2015 12:17 PM, Martin Basti wrote:
On 09.12.2015 12:10, Martin Basti wrote:
On 09.12.2015 11:14, Oleg Fayans wrote:
On 12/09/2015 10:30 AM, Martin Basti wrote:
On 08.12.2015 23:48, Oleg Fayans wrote:
Substituted a hardcoded suffix name with a constant DOMAIN_SUFFIX_NAME
On 12/08/2015 02:33 PM, Oleg Fayans wrote:
The patches are rebased against the current master.
On 12/02/2015 05:10 PM, Martin Basti wrote:
On 02.12.2015 16:18, Oleg Fayans wrote:
On 12/01/2015 04:08 PM, Martin Basti wrote:
On 27.11.2015 16:26, Oleg Fayans wrote:
And patch N 16 passes lint too:
On 11/27/2015 04:03 PM, Oleg Fayans wrote:
On 11/27/2015 03:26 PM, Martin Basti wrote:
On 27.11.2015 15:04, Oleg Fayans wrote:
All your suggestions were taken into account. Both patches are
updated. Thank you for your help!
On 11/26/2015 10:50 AM, Martin Basti wrote:
On 26.11.2015 10:04, Oleg Fayans wrote:
I agree to all your points but one. please, see my comment
On 11/25/2015 07:42 PM, Martin Basti wrote:
Please be aware of
Please do not use MIN and MAX_DOMAIN_LEVEL constants,
over time, use DOMAIN_LEVEL_0 and DOMAIN_LEVEL_1 for domain
Why uninstall KRA then server, is not enough just uninstall
covers KRA uninstall?
+ def teardown_method(self, method):
+ for host in self.replicas:
Can be this function more generic? It should allow specify
KRA should be installed not just master
+ def test_kra_install_master(self):
Can be the test name more specific, something like
please remove this, the patch is on review and it will be
+ @pytest.mark.xfail # Ticket N 5455
and as I mentioned in ticket #5455, I cannot reproduce
ipa-kra-install, so please provide steps to reproduce if
this still does not work as expected with KRA.
6) This is completely wrong, it removes everything that we
achieve with previous patches with domain level in CI
Actually, being able to configure domain level per class
convenient, than to always have to think which domain
appropriate for which particular test during jenkins job
configuration. In fact, I should have thought about it
beginning. For example, in test_replica_promotion.py we
which intiates with domain level = 1, while others - with
0. With config-based approach, we would have to implement a
step that raises domain level. Overall, I am against the
when you have to remember to set certain domain level in
any particular test. The tests themselves should be aware of
domain level they need.
I do not say that we should not have something that overrides
in from config in a particular test case, I say your patch is
I agree it is useful to have param domain_level in
intall_topo methods, but is cannot be MAX_DOMAIN_LEVEL by
because with your current patch the domain_level in config is
at all, it will be always MAX_DOMAIN_LEVEL
For example I want to achieve this goal:
test_vault.py, this test suite can run on domain level1
level0, so with one test we can test 2 domain levels just
domain level into config file.
I agree that with extraordinary test like replica
need something that allows override the config file.
As I said bellow, domain_level default value should be
install_master and install_topo plugin. If domain level was
use the specified one, if not (value is None) use the domain
[PATCH] Enabled setting domain_level per class derived from
When I configure domain level 0 in yaml config, how is this
get into install methods when you removed that code?
- "--domain-level=%i" % host.config.domain_level
+ "--domain-level=%i" % domain_level
You always use MAX_DOMAIN_LEVEL in this case or whatever is
I suggest to use domain_level=None, and when it is None use
'host.config.domain_level', if it is not None, use
With this we can specify domain level in config file for
be used for both domain levels and you can manually specify
for test that requires specific domain level.
Also this should go away
def install(cls, mh):
+ if hasattr(cls, "domain_level") and cls.master:
+ cls.master.config.domain_level = cls.domain_level
if cls.topology is None:
I do not see reason why test should override
On 25.11.2015 16:44, Oleg Fayans wrote:
Here is the updated version of the patch (more tests +
issues of the first one) + patch 0017, that implements the
changes in the background code, i. e. patch 16 does not
On 11/18/2015 05:20 PM, Martin Basti wrote:
On 09.11.2015 15:09, Oleg Fayans wrote:
Here are first two automated testcases from this (so far
Testplan review is highly appreciated
PATCH 16: NACK
What is the reason to add an unused parameter to
Also it is good practise to add new option as the last
cab you in both tests specify a domain level with
both test call install_topo with custom domain level,
because 1) (did you run the test?)
How the test "TestLevel1" is supposed to work?
Respectively why there is call of install_topo() that
As this test just tests that ipa-replica-prepare is not
is it worth to spend 20 minutes with installing
tot use it? IMO to install master in install step is
************* Module ipatests.test_integration.base
IntegrationTest.install] Class 'IntegrationTest' has no
[E1101(no-member), Dummy.install] Class 'Dummy' has no
Module 'ipatests.test_integration.tasks' has no 'setup_replica'
Is it so hard to run pylint before patch is posted for review?
Sorry, my bad. Fixed.
Why is this change in the patch?
- # Clean up the test directory
- host.run_command(['rm', '-rvf', host.config.test_dir])
Otherwise 2 out of 8 tests fail with IOError at line 78 of
I do not understand yet how does this happen, but if you remove
ipatests folder once, it then fails to be created again.
So this should be in separated patch and investigated properly.
is enough to have this check only in install_master,
pass None to install_master
+ if domain_level is None:
+ domain_level = master.config.domain_level
IMO replica-manage del should cleanup hosts entry, so following
should not be needed.
+ if cleanhost:
+ master.run_command(["ipa", "host-del", "--updatedns",
Well, in fact it does not. At least the corresponding dns record
and causes the subsequent ipa-client-install to fail. Probably
bug. On the other hand, if I promote an existing client to
then delete this replica, then, I probably want the host record
was created during client-install) to stay in the system. So,
look like a bug to me.
No you don't, because replica uninstallation also removes the
I tried it with ipa42, ipa-replica-manage del removes host entry,
DNS A records, only SSHFP records stay there (I'm not sure if it
Also I received this message
Failed to cleanup replica1.ipa.test DNS entries: no matching entry
You may need to manually remove them from the tree
But, A record has been removed, so this is probably false
it needs to have a ticket.
Agree, that was an issue with my setup.
This variable is not used
+ kra_uninstall = ["ipa-kra-install", "--uninstall", "-U"]
Why do you need this
+ kra_install = ["ipa-kra-install", "-U", "-p",
when you implemented tasks.install_kra that returns the exactly
./ipatests/test_integration/tasks.py:928:1: E302 expected 2 blank
./ipatests/test_integration/tasks.py:934:1: E302 expected 2 blank
./ipatests/test_integration/tasks.py:939:1: E302 expected 2 blank
./ipatests/test_integration/tasks.py:943:1: E302 expected 2 blank
./ipatests/test_integration/tasks.py:950:80: E501 line too long
(80 > 79
too long (85 > 79 characters)
too long (85 > 79 characters)
too long (88 > 79 characters)
too long (80 > 79 characters)
too long (83 > 79 characters)
too long (81 > 79 characters)
too long (80 > 79 characters)
too long (82 > 79 characters)
too long (80 > 79 characters)
Most of these complaints are unrelated to the current patches.
It's better to create a separate patch addressing PEP8 errors.
I beg for your pardon, those PEP8 errors have been introduced by
Why this must be stored in instance? IMO to have it stored as
variable is perfect
This patch is missing something.
I am sorry, I forgot to revert my previous change. The correct patch is
************* Module ipatests.test_integration.test_replica_promotion
[E1123(unexpected-keyword-arg), Dummy.install] Unexpected keyword
argument 'domain_level' in function call)
[E1101(no-member), Dummy.install] Class 'Dummy' has no 'domain_level'
[E1101(no-member), Dummy.teardown_method] Module
'ipatests.test_integration.tasks' has no 'uninstall_replica' member)
Module 'ipatests.test_integration.tasks' has no 'ipa_backup' member)
Module 'ipatests.test_integration.tasks' has no 'ipa_restore' member)
[E1123(unexpected-keyword-arg), TestCAInstall.install] Unexpected
keyword argument 'domain_level' in function call)
Sorry I forgot to apply patch 17, my bad, I'm continuing with review
Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code