Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 29.10.2015 18:32, Martin Basti wrote:



On 29.10.2015 18:31, Martin Basti wrote:

NACK

1)
DO NOT use tabs in code to indent

2)
Replica uninstallation does not work, uninstallation works different 
with domain level 0 and 1 (currently uninstallation with domain 1 
level will not work, it is known issue, but still the patch should 
solve the uninstallation)
This is not valid, my bad, I was confused with new behaviour of replica 
uninstallation, but it is bug not a feature.

So replica uninstallation is the same for level 0 and 1
Sorry.


3)
apply_common_fixes(host)
Method for domain_level 1 is called twice, first time in replica 
install, second time in client install


4)
during testing this patch I used test_simple_replication and I found 
4 bugs:

3 bugs -^^^

#5419, #5420, #5421
IMO it is related only to this one test case and to pass this test 
case #5419 or #5421 must be fixed.



On 27.10.2015 16:34, Oleg Fayans wrote:

Hi Martin,

The updated patch is attached

On 10/27/2015 01:58 PM, Martin Basti wrote:



On 27.10.2015 13:56, Oleg Fayans wrote:



On 10/27/2015 01:22 PM, Martin Basti wrote:



On 27.10.2015 12:06, Oleg Fayans wrote:

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my
comments
below
My comments inline, I may be completely wrong in how the test 
suite

work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to
this
ticket


done



2)
-def install_master(host, setup_dns=True, 
setup_kra=False):

+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will 
use the

default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain 
level is

independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain 
level for

which
test will be running, we were talking about using config
files,
but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which
tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would
require it.
All the existing tests just use install_replica
 method, no matter how is it done.
How about topology CI test? This can be executed only with 
domain

level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap 
file,

sorry
about that).


1, right?




5)
+ '--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" %
host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed 
that the

default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the 
pytest.fixture

needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants MAX_DOMAIN_LEVEL as default, 
for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this 
part

you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set 
to 0 in
config.yaml. This default value will always override the 
explicit

one.
wat? in case that kwargs is dict containing values from config 
file:


In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)




Respectively you should use this:

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

Hi Martin,

On 11/02/2015 10:39 AM, Martin Basti wrote:



On 29.10.2015 18:32, Martin Basti wrote:



On 29.10.2015 18:31, Martin Basti wrote:

NACK

1)
DO NOT use tabs in code to indent

Fixed


2)
Replica uninstallation does not work, uninstallation works different
with domain level 0 and 1 (currently uninstallation with domain 1
level will not work, it is known issue, but still the patch should
solve the uninstallation)

This is not valid, my bad, I was confused with new behaviour of replica
uninstallation, but it is bug not a feature.
So replica uninstallation is the same for level 0 and 1
Sorry.


3)
apply_common_fixes(host)
Method for domain_level 1 is called twice, first time in replica
install, second time in client install

Fixed


4)
during testing this patch I used test_simple_replication and I found
4 bugs:

3 bugs -^^^

#5419, #5420, #5421

Bug #5419 fixed, see patch N 15


IMO it is related only to this one test case and to pass this test
case #5419 or #5421 must be fixed.


On 27.10.2015 16:34, Oleg Fayans wrote:

Hi Martin,

The updated patch is attached

On 10/27/2015 01:58 PM, Martin Basti wrote:



On 27.10.2015 13:56, Oleg Fayans wrote:



On 10/27/2015 01:22 PM, Martin Basti wrote:



On 27.10.2015 12:06, Oleg Fayans wrote:

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my
comments
below

My comments inline, I may be completely wrong in how the test
suite
work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to
this
ticket


done



2)
-def install_master(host, setup_dns=True,
setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will
use the
default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain
level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain
level for
which
test will be running, we were talking about using config
files,
but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which
tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would
require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with
domain
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap
file,
sorry
about that).


1, right?




5)
+ '--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" %
host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed
that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the
pytest.fixture
needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants MAX_DOMAIN_LEVEL as default,
for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this
part
you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set
to 0 in
config.yaml. This default value will always override the
explicit
one.

wat? in case that kwargs is dict containing values from config
file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 29.10.2015 18:31, Martin Basti wrote:

NACK

1)
DO NOT use tabs in code to indent

2)
Replica uninstallation does not work, uninstallation works different 
with domain level 0 and 1 (currently uninstallation with domain 1 
level will not work, it is known issue, but still the patch should 
solve the uninstallation)


3)
apply_common_fixes(host)
Method for domain_level 1 is called twice, first time in replica 
install, second time in client install


4)
during testing this patch I used test_simple_replication and I found 4 
bugs:

3 bugs -^^^

#5419, #5420, #5421
IMO it is related only to this one test case and to pass this test 
case #5419 or #5421 must be fixed.



On 27.10.2015 16:34, Oleg Fayans wrote:

Hi Martin,

The updated patch is attached

On 10/27/2015 01:58 PM, Martin Basti wrote:



On 27.10.2015 13:56, Oleg Fayans wrote:



On 10/27/2015 01:22 PM, Martin Basti wrote:



On 27.10.2015 12:06, Oleg Fayans wrote:

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my
comments
below
My comments inline, I may be completely wrong in how the test 
suite

work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to
this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will 
use the

default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain 
level is

independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level 
for

which
test will be running, we were talking about using config
files,
but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which
tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would
require it.
All the existing tests just use install_replica
 method, no matter how is it done.
How about topology CI test? This can be executed only with 
domain

level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file,
sorry
about that).


1, right?




5)
+ '--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" %
host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed 
that the

default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the 
pytest.fixture

needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, 
for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part
you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 
0 in

config.yaml. This default value will always override the explicit
one.
wat? in case that kwargs is dict containing values from config 
file:


In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)




Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

Now that would definitely not work in case of domain_level is 0:
0 or 1 is always 1

Oh right.

I had discussion with Tomas, and we should add there check if it 
is 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

Hi Martin,

The updated version of the patch is attached. Please, see my comments below

On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file, sorry
about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture needs
to know the value of domain level before we can do any dynamic detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in 
config.yaml. This default value will always override the explicit one.




freeipa-tests depends on freeipa-python so the constants should be
available in tests.

So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)


This would not work if domainlevel is not set in config.yaml, in which 
case the host.config.domain_level is None.




4)
Please add comment to function +def domainlevel(host): that it is useful
for test where domain level will be raised dynamically, otherwise it may
be lost after test refactoring as somebody may consider it as unneeded
and replace it with config dict.

Done



So summary is the 1) and 2) are replaced by 3) :)

Martin^2


--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 385ad3ca9c564e3d08a0a3256dfc65ab07374a04 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Tue, 27 Oct 2015 09:43:33 +0100
Subject: [PATCH] Updated the tests according to the new replica installation
 workflow

As of 4.3 the replica installation is performed without preparing a gpg file on
master, but rather enrolling a future replica as a client with subsequent
promotion of the client. This required the corresponding change in the
integration tests

https://fedorahosted.org/freeipa/ticket/5379
---
 ipatests/test_integration/config.py  |  3 +-
 ipatests/test_integration/tasks.py   | 41 
 ipatests/test_integration/test_testconfig.py |  5 +++-
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/ipatests/test_integration/config.py b/ipatests/test_integration/config.py
index 785a9bb8c420f99980c098887e0bd31422119564..60a4bd700afe3027dfcbdf203d02373f8a7aa9f9 100644
--- a/ipatests/test_integration/config.py
+++ 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my comments 
below
My comments inline, I may be completely wrong in how the test suite 
work, so feel free to correct me.


Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the 
default

domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, 
respectively

new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, 
but

feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the 
directive

domain_level



4)
Did you resolve the pytest fixtures which specifies which tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it.
All the existing tests just use install_replica
 method, no matter how is it done.
How about topology CI test? This can be executed only with domain 
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file, sorry
about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: 
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in 
function

call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture 
needs
to know the value of domain level before we can do any dynamic 
detection

on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level', 
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in 
config.yaml. This default value will always override the explicit one.

wat? in case that kwargs is dict containing values from config file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

because kwargs IMO contains undefined config values as None







freeipa-tests depends on freeipa-python so the constants should be
available in tests.

So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)


This would not work if domainlevel is not set in config.yaml, in 
which case the host.config.domain_level is None.
Domain level will never be None because self.domain_level = 
kwargs.get('domain_level', MAX_DOMAIN_LEVEL)






4)
Please add comment to function +def domainlevel(host): that it is 
useful
for test where domain level will be raised dynamically, otherwise it 
may

be lost after test refactoring as somebody may consider it as unneeded
and replace it with config dict.

Done



So summary is the 1) and 2) are replaced by 3) :)

Martin^2






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

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my comments 
below
My comments inline, I may be completely wrong in how the test suite 
work, so feel free to correct me.


Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the 
default

domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, 
respectively

new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it.
All the existing tests just use install_replica
 method, no matter how is it done.
How about topology CI test? This can be executed only with domain 
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file, sorry
about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: 
[E1123(unexpected-keyword-arg),

allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture needs
to know the value of domain level before we can do any dynamic detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level', 
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in 
config.yaml. This default value will always override the explicit one.

wat? in case that kwargs is dict containing values from config file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0





freeipa-tests depends on freeipa-python so the constants should be
available in tests.

So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)


This would not work if domainlevel is not set in config.yaml, in which 
case the host.config.domain_level is None.
Domain level will never be None because self.domain_level = 
kwargs.get('domain_level', MAX_DOMAIN_LEVEL)






4)
Please add comment to function +def domainlevel(host): that it is useful
for test where domain level will be raised dynamically, otherwise it may
be lost after test refactoring as somebody may consider it as unneeded
and replace it with config dict.

Done



So summary is the 1) and 2) are replaced by 3) :)

Martin^2




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

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 27.10.2015 13:56, Oleg Fayans wrote:



On 10/27/2015 01:22 PM, Martin Basti wrote:



On 27.10.2015 12:06, Oleg Fayans wrote:

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my 
comments

below

My comments inline, I may be completely wrong in how the test suite
work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to 
this

ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the
default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for
which
test will be running, we were talking about using config 
files,

but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which 
tests

can be
run under which domain level?


In fact, we do not seem to have any tests yet that would
require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file,
sorry
about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % 
host.config.domain_level)


always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture
needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part 
you

need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in
config.yaml. This default value will always override the explicit 
one.

wat? in case that kwargs is dict containing values from config file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)




Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

Now that would definitely not work in case of domain_level is 0:
0 or 1 is always 1

Oh right.

I had discussion with Tomas, and we should add there check if it is not
None, in case that kwargs contains {'domain_level': None} (One does not
know with test when the None value appears there)


I do not get this. If we do not specify domain_level in config.yaml, 
it will automatically be populated with a MAX_DOMAIN_LEVEL value. That 
means domain_level will never ever possibly be None.
I do not know how pytest works inside, if you are 100% sure, that the 
case written above cannot happen, I'm fine with it.

Martin




self.domain_level = kwargs.get('domain_level')
if self.domain_level is None:
 self.domain_level = MAX_DOMAIN_LEVEL

As we cannot user 'or' expression with integers, so this is the right 
way.




because kwargs IMO contains undefined config values as None







freeipa-tests depends on freeipa-python so the constants should be
available in 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

On 10/27/2015 01:22 PM, Martin Basti wrote:



On 27.10.2015 12:06, Oleg Fayans wrote:

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my comments
below

My comments inline, I may be completely wrong in how the test suite
work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the
default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for
which
test will be running, we were talking about using config files,
but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would
require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file,
sorry
about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture
needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in
config.yaml. This default value will always override the explicit one.

wat? in case that kwargs is dict containing values from config file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)




Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

Now that would definitely not work in case of domain_level is 0:
0 or 1 is always 1

Oh right.

I had discussion with Tomas, and we should add there check if it is not
None, in case that kwargs contains {'domain_level': None} (One does not
know with test when the None value appears there)


I do not get this. If we do not specify domain_level in config.yaml, it 
will automatically be populated with a MAX_DOMAIN_LEVEL value. That 
means domain_level will never ever possibly be None.




self.domain_level = kwargs.get('domain_level')
if self.domain_level is None:
 self.domain_level = MAX_DOMAIN_LEVEL

As we cannot user 'or' expression with integers, so this is the right way.




because kwargs IMO contains undefined config values as None







freeipa-tests depends on freeipa-python so the constants should be
available in tests.

So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)


This would 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my comments
below

My comments inline, I may be completely wrong in how the test suite
work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the
default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files,
but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file, sorry
about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture
needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in
config.yaml. This default value will always override the explicit one.

wat? in case that kwargs is dict containing values from config file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)




Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

Now that would definitely not work in case of domain_level is 0:
0 or 1 is always 1



because kwargs IMO contains undefined config values as None







freeipa-tests depends on freeipa-python so the constants should be
available in tests.

So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)


This would not work if domainlevel is not set in config.yaml, in
which case the host.config.domain_level is None.

Domain level will never be None because self.domain_level =
kwargs.get('domain_level', MAX_DOMAIN_LEVEL)





4)
Please add comment to function +def domainlevel(host): that it is
useful
for test where domain level will be raised dynamically, otherwise it
may
be lost after test refactoring as somebody may consider it as unneeded
and replace it with config dict.

Done



So summary is the 1) and 2) are replaced by 3) :)

Martin^2








--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 7ebc7c0899a2437463aaf7fdc03ea93fbab057bb Mon Sep 17 00:00:00 2001
From: Oleg Fayans 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 27.10.2015 12:06, Oleg Fayans wrote:

Hi Martin,

On 10/27/2015 10:50 AM, Martin Basti wrote:



On 27.10.2015 10:22, Martin Basti wrote:



On 27.10.2015 10:00, Oleg Fayans wrote:

Hi Martin,

The updated version of the patch is attached. Please, see my comments
below

My comments inline, I may be completely wrong in how the test suite
work, so feel free to correct me.

Martin



On 10/26/2015 06:48 PM, Martin Basti wrote:



On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the
default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3,
respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for 
which

test will be running, we were talking about using config files,
but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the
directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests
can be
run under which domain level?


In fact, we do not seem to have any tests yet that would 
require it.

All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain
level


That's right. The topology test was updated. Patch is attached
together with a proper version of 11-th patch (not a swap file, 
sorry

about that).


1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85:
[E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in
function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the
default level is the one which is default in tested package.
These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture
needs
to know the value of domain level before we can do any dynamic
detection
on master.

So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Done


Also I'm not sure if the values are inherited from the
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you
need default value, or the fixture will not work as expected.
+self.domain_level = kwargs.get('domain_level',
MAX_DOMAIN_LEVEL)


This won't work in cases when domainlevel is explicitly set to 0 in
config.yaml. This default value will always override the explicit one.

wat? in case that kwargs is dict containing values from config file:

In [1]: kwargs = dict(domain_level=0)

In [2]: kwargs.get('domain_level', 123)
Out[2]: 0


Yep, right you are, it works as expected. Now the line looks like:
self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)




Respectively you should use this:

self.domain_level = kwargs.get('domain_level')or MAX_DOMAIN_LEVEL

Now that would definitely not work in case of domain_level is 0:
0 or 1 is always 1

Oh right.

I had discussion with Tomas, and we should add there check if it is not 
None, in case that kwargs contains {'domain_level': None} (One does not 
know with test when the None value appears there)


self.domain_level = kwargs.get('domain_level')
if self.domain_level is None:
self.domain_level = MAX_DOMAIN_LEVEL

As we cannot user 'or' expression with integers, so this is the right way.




because kwargs IMO contains undefined config values as None







freeipa-tests depends on freeipa-python so the constants should be
available in tests.

So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)


This would not work if domainlevel is not set in config.yaml, in
which case the host.config.domain_level is None.

Domain level will never be None because self.domain_level =
kwargs.get('domain_level', MAX_DOMAIN_LEVEL)





4)
Please add comment to 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain level


That's right. The topology test was updated. Patch is attached together 
with a proper version of 11-th patch (not a swap file, sorry about that).



1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function
call)


Fixed










--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 7a0da0e4220a2e2b0e1475a90c69f9fdcee43210 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Fri, 23 Oct 2015 14:58:24 +0200
Subject: [PATCH] Updated the tests according to the new replica installation
 workflow

As of 4.3 the replica installation is performed without preparing a gpg file on
master, but rather enrolling a future replica as a client with subsequent
promotion of the client. This required the corresponding change in the
integration tests

https://fedorahosted.org/freeipa/ticket/5379
---
 ipatests/test_integration/config.py  |  3 ++-
 ipatests/test_integration/tasks.py   | 40 +++-
 ipatests/test_integration/test_testconfig.py |  4 ++-
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/ipatests/test_integration/config.py b/ipatests/test_integration/config.py
index 785a9bb8c420f99980c098887e0bd31422119564..60a4bd700afe3027dfcbdf203d02373f8a7aa9f9 100644
--- a/ipatests/test_integration/config.py
+++ b/ipatests/test_integration/config.py
@@ -39,6 +39,7 @@ class Config(pytest_multihost.config.Config):
 'ad_admin_name',
 'ad_admin_password',
 'dns_forwarder',
+'domain_level',
 }
 
 def __init__(self, **kwargs):
@@ -56,7 +57,7 @@ class Config(pytest_multihost.config.Config):
 '%s.pool.ntp.org' % random.randint(0, 3)))
 self.ad_admin_name = kwargs.get('ad_admin_name') or 'Administrator'
 self.ad_admin_password = kwargs.get('ad_admin_password') or 'Secret123'
-
+self.domain_level = kwargs.get('domain_level')
 # 8.8.8.8 is probably the best-known public DNS
 self.dns_forwarder = kwargs.get('dns_forwarder') or '8.8.8.8'
 self.debug = False
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index e241454a984aac97eb2d0955f55bb83d85bf9d4c..684edeb04b008d3b9ec1cefe2929a8f6167fccb4 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -79,6 +79,12 @@ def prepare_host(host):
 host.put_file_contents(env_filename, env_to_script(host.to_env()))
 
 
+def allow_sync_ptr(host):
+kinit_admin(host)
+host.run_command(["ipa", "dnsconfig-mod", "--allow-sync-ptr=true"],
+ raiseonerr=False)
+
+
 def apply_common_fixes(host):
 fix_etc_hosts(host)
 fix_hostname(host)
@@ -263,6 +269,9 @@ def install_master(host, setup_dns=True, setup_kra=False):
 '-a', host.config.admin_password
 ]
 
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)
+
 if setup_dns:
 args.extend([
 '--setup-dns',
@@ -288,6 +297,16 @@ def get_replica_filename(replica):
 return os.path.join(replica.config.test_dir, 'replica-info.gpg')
 
 
+def domainlevel(host):
+result = host.run_command(['ipa', 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 26.10.2015 08:59, Oleg Fayans wrote:



On 10/23/2015 03:10 PM, Martin Basti wrote:



On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this 
ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False,
domainlevel=1):

I suggest to use default domainlevel=None, which will use the default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive
domain_level



4)
Did you resolve the pytest fixtures which specifies which tests 
can be

run under which domain level?


In fact, we do not seem to have any tests yet that would require it.
All the existing tests just use install_replica
 method, no matter how is it done.

How about topology CI test? This can be executed only with domain level


That's right. The topology test was updated. Patch is attached 
together with a proper version of 11-th patch (not a swap file, sorry 
about that).



1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function
call)


Fixed












1)
+if not host.config.domain_level == None:
+args.append("--domain-level=%i" % host.config.domain_level)

always use: variable *is not None*

2)
Why there is specified level 1 as default? IIRC we agreed that the 
default level is the one which is default in tested package.

These should be None and "":
+"domain_level": "1"

+"DOMAINLVL": "1",

3)
However, as I read the patch 12, and I'm right, the pytest.fixture needs 
to know the value of domain level before we can do any dynamic detection 
on master.


So we should use the constants  MAX_DOMAIN_LEVEL as default, for 2)

Also I'm not sure if the values are inherited from the 
DEFAULT_OUTPUT_DICT to code, I think it is not, so for this part you 
need default value, or the fixture will not work as expected.

+self.domain_level = kwargs.get('domain_level', MAX_DOMAIN_LEVEL)

freeipa-tests depends on freeipa-python so the constants should be 
available in tests.


So then you also need update this line

+if not host.config.domain_level != MAX_DOMAIN_LEVEL:
+args.append("--domain-level=%i" % host.config.domain_level)

4)
Please add comment to function +def domainlevel(host): that it is useful 
for test where domain level will be raised dynamically, otherwise it may 
be lost after test refactoring as somebody may consider it as unneeded 
and replace it with config dict.


So summary is the 1) and 2) are replaced by 3) :)

Martin^2

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

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, 
domainlevel=1):


I suggest to use default domainlevel=None, which will use the default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive 
domain_level




4)
Did you resolve the pytest fixtures which specifies which tests can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it. 
All the existing tests just use install_replica

 method, no matter how is it done.



5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function 
call)


Fixed








You did not send the patch but a swap file.

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

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, domainlevel=1):

I suggest to use default domainlevel=None, which will use the default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive 
domain_level




4)
Did you resolve the pytest fixtures which specifies which tests can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it. All 
the existing tests just use install_replica

 method, no matter how is it done.



5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function call)


Fixed






--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.


.freeipa-ofayans-0011.1-replica-promotion-changes-in-tests.patch.swp
Description: Binary data
-- 
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

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 23.10.2015 15:00, Oleg Fayans wrote:

Hi Martin,

Here comes the updated version.

On 10/22/2015 05:38 PM, Martin Basti wrote:



On 22.10.2015 15:23, Martin Basti wrote:


On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this ticket


done



2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, 
domainlevel=1):


I suggest to use default domainlevel=None, which will use the default
domain level (specified in build)


done



3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively
new IPA must support all older domain levels, domain level is
independent on IPA version, only admin can raise it up.

So you have to find out way how to pass the domain level for which
test will be running, we were talking about using config files, but
feel free to find something new and better


Fixed. Now, we declare domain level in config.yaml with the directive 
domain_level




4)
Did you resolve the pytest fixtures which specifies which tests can be
run under which domain level?


In fact, we do not seem to have any tests yet that would require it. 
All the existing tests just use install_replica

 method, no matter how is it done.
How about topology CI test? This can be executed only with domain level 
1, right?




5)
+'--ip-address', client.ip,

why this change to client install?


Right, it found to be unnecessary.



Martin^2



6)
* Module ipatests.test_integration.tasks
ipatests/test_integration/tasks.py:85: [E1123(unexpected-keyword-arg),
allow_sync_ptr] Unexpected keyword argument 'raiseonerr' in function 
call)


Fixed








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

[Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Oleg Fayans]

--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From c0a41ec0fbfb70ac33fd97beb1641ecfa126a87d Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Thu, 22 Oct 2015 14:05:24 +0200
Subject: [PATCH] Updated the tests according to the new replica installation
 workflow

As of 4.3 the replica installation is performed without preparing a gpg file on
master, but rather enrolling a future replica as a client with subsequent
promotion of the client. This required the corresponding change in the
integration tests

https://fedorahosted.org/freeipa/ticket/5379
---
 ipatests/test_integration/tasks.py | 50 +-
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index e241454a984aac97eb2d0955f55bb83d85bf9d4c..02daa2ad768d24ff3d4e781f4e286e2f926eeac7 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -61,12 +61,14 @@ def check_arguments_are(slice, instanceof):
 return wrapped
 return wrapper
 
+
 def prepare_reverse_zone(host, ip):
 zone = get_reverse_zone_default(ip)
 host.run_command(["ipa",
   "dnszone-add",
   zone], raiseonerr=False)
 
+
 def prepare_host(host):
 if isinstance(host, Host):
 env_filename = os.path.join(host.config.test_dir, 'env.sh')
@@ -79,6 +81,12 @@ def prepare_host(host):
 host.put_file_contents(env_filename, env_to_script(host.to_env()))
 
 
+def allow_sync_ptr(host):
+kinit_admin(host, raiseonerr=False)
+host.run_command(["ipa", "dnsconfig-mod", "--allow-sync-ptr=true"],
+ raiseonerr=False)
+
+
 def apply_common_fixes(host):
 fix_etc_hosts(host)
 fix_hostname(host)
@@ -246,7 +254,7 @@ def enable_replication_debugging(host):
  stdin_text=logging_ldif)
 
 
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, domainlevel=1):
 host.collect_log(paths.IPASERVER_INSTALL_LOG)
 host.collect_log(paths.IPACLIENT_INSTALL_LOG)
 inst = host.domain.realm.replace('.', '-')
@@ -260,7 +268,8 @@ def install_master(host, setup_dns=True, setup_kra=False):
 'ipa-server-install', '-U',
 '-r', host.domain.name,
 '-p', host.config.dirman_password,
-'-a', host.config.admin_password
+'-a', host.config.admin_password,
+'--domain-level=%i' % domainlevel
 ]
 
 if setup_dns:
@@ -288,6 +297,16 @@ def get_replica_filename(replica):
 return os.path.join(replica.config.test_dir, 'replica-info.gpg')
 
 
+def domainlevel(host):
+result = host.run_command(['ipa', 'domainlevel-get'], raiseonerr=False)
+level = 0
+domlevel_re = re.compile('.*(\d)')
+if result.returncode == 0:
+# "domainlevel-get" command doesn't exist on ipa versions prior to 4.3
+level = int(domlevel_re.findall(result.stdout_text)[0])
+return level
+
+
 def replica_prepare(master, replica):
 apply_common_fixes(replica)
 fix_apache_semaphores(replica)
@@ -306,15 +325,25 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
 setup_kra=False):
 replica.collect_log(paths.IPAREPLICA_INSTALL_LOG)
 replica.collect_log(paths.IPAREPLICA_CONNCHECK_LOG)
-
-replica_prepare(master, replica)
-
-replica_filename = get_replica_filename(replica)
+allow_sync_ptr(master)
+# Otherwise ipa-client-install would not create a PTR
+# and replica installation would fail
+apply_common_fixes(replica)
+fix_apache_semaphores(replica)
+domain_level = domainlevel(master)
 args = ['ipa-replica-install', '-U',
 '-p', replica.config.dirman_password,
 '-w', replica.config.admin_password,
-'--ip-address', replica.ip,
-replica_filename]
+'--ip-address', replica.ip]
+if domain_level == 0:
+# prepare the replica file on master and put it to replica, AKA "old way"
+	replica_prepare(master, replica)
+replica_filename = get_replica_filename(replica)
+args.append(replica_filename)
+else:
+# install client on a replica machine and then promote it to replica
+install_client(master, replica)
+
 if setup_ca:
 args.append('--setup-ca')
 if setup_dns:
@@ -326,7 +355,6 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
 
 enable_replication_debugging(replica)
 setup_sssd_debugging(replica)
-
 if setup_kra:
 assert setup_ca, "CA must be installed on replica with KRA"
 args = [
@@ -336,7 +364,6 @@ def install_replica(master, replica, setup_ca=True, setup_dns=False,
 "-U",
 ]
 replica.run_command(args)
-
 kinit_admin(replica)
 
 
@@ -344,15 +371,14 @@ def install_client(master, client, extra_args=()):
 

Re: [Freeipa-devel] [PATCH 0011] Replica promotion related changes in integration tests

[Martin Basti]

On 22.10.2015 14:13, Oleg Fayans wrote:






Hello,

thank you for the patch.

1)
please remove the added empty lines, they are unrelated to this ticket

2)
-def install_master(host, setup_dns=True, setup_kra=False):
+def install_master(host, setup_dns=True, setup_kra=False, domainlevel=1):

I suggest to use default domainlevel=None, which will use the default 
domain level (specified in build)


3)
+domain_level = domainlevel(master)
I do not think that this meets expectations.

We have to test, both domain level 0 and 1 for IPA 4.3, respectively new 
IPA must support all older domain levels, domain level is independent on 
IPA version, only admin can raise it up.


So you have to find out way how to pass the domain level for which test 
will be running, we were talking about using config files, but feel free 
to find something new and better


4)
Did you resolve the pytest fixtures which specifies which tests can be 
run under which domain level?


5)
+'--ip-address', client.ip,

why this change to client install?

Martin^2

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