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