Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/18/2016 04:59 PM, Lenka Doudova wrote:
On 07/18/2016 04:55 PM, Martin Babinsky wrote:
On 07/14/2016 11:42 AM, Lenka Doudova wrote:
On 07/13/2016 05:40 PM, Martin Babinsky wrote:
On 07/01/2016 04:15 PM, Lenka Doudova wrote:
On 07/01/2016 02:38 PM, Martin Babinsky wrote:
On 07/01/2016 06:36 AM, Lenka Doudova wrote:
On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used
only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable
me to
reuse existing function without copy-pasting them for one variable
change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing
magic
inside. The same goes with `remove trust_with_ad`. You may want to
fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it
even
needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the
base test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole
test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master,
cls.ad_subdomain)
# no need to re-implement the function just to get to the
child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
Hi,
thanks for review, fixed patch attached.
Lenka
Hi Lenka,
I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.
I am attaching a quick untested patch illustrating how the
implementation should look like.
Also you have some leftovers from previous revision, please fix them:
Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/18/2016 04:55 PM, Martin Babinsky wrote:
On 07/14/2016 11:42 AM, Lenka Doudova wrote:
On 07/13/2016 05:40 PM, Martin Babinsky wrote:
On 07/01/2016 04:15 PM, Lenka Doudova wrote:
On 07/01/2016 02:38 PM, Martin Babinsky wrote:
On 07/01/2016 06:36 AM, Lenka Doudova wrote:
On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used
only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable
me to
reuse existing function without copy-pasting them for one variable
change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing
magic
inside. The same goes with `remove trust_with_ad`. You may want to
fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it
even
needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the
base test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole
test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master,
cls.ad_subdomain)
# no need to re-implement the function just to get to the
child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
Hi,
thanks for review, fixed patch attached.
Lenka
Hi Lenka,
I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.
I am attaching a quick untested patch illustrating how the
implementation should look like.
Also you have some leftovers from previous revision, please fix them:
Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137:
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/13/2016 05:40 PM, Martin Babinsky wrote:
On 07/01/2016 04:15 PM, Lenka Doudova wrote:
On 07/01/2016 02:38 PM, Martin Babinsky wrote:
On 07/01/2016 06:36 AM, Lenka Doudova wrote:
On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used
only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable
me to
reuse existing function without copy-pasting them for one variable
change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing
magic
inside. The same goes with `remove trust_with_ad`. You may want to
fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it even
needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the
base test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
# no need to re-implement the function just to get to the child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
Hi,
thanks for review, fixed patch attached.
Lenka
Hi Lenka,
I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.
I am attaching a quick untested patch illustrating how the
implementation should look like.
Also you have some leftovers from previous revision, please fix them:
Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137: recipe for target 'lint' failed
make: *** [lint] Error 1
sending incremental file list
(this is
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/01/2016 02:38 PM, Martin Babinsky wrote:
On 07/01/2016 06:36 AM, Lenka Doudova wrote:
On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust
feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable me to
reuse existing function without copy-pasting them for one variable
change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(),
subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing magic
inside. The same goes with `remove trust_with_ad`. You may want to fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it even
needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the
base test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad =
cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
# no need to re-implement the function just to get to the child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
Hi,
thanks for review, fixed patch attached.
Lenka
Hi Lenka,
I am still not happy about the patch underutilizing the potential for
a proper inheritance hierarchy and instead relying on a staggering
amounts of copy-pasting for implementation.
I am attaching a quick untested patch illustrating how the
implementation should look like.
Also you have some leftovers from previous revision, please fix them:
Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137: recipe for target 'lint' failed
make: *** [lint] Error 1
sending incremental file list
(this is before I started meddling with it)
Thank you very much for the example, fixed
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 07/01/2016 06:36 AM, Lenka Doudova wrote:
On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable me to
reuse existing function without copy-pasting them for one variable
change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing magic
inside. The same goes with `remove trust_with_ad`. You may want to fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it even
needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the
base test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
# no need to re-implement the function just to get to the child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
Hi,
thanks for review, fixed patch attached.
Lenka
Hi Lenka,
I am still not happy about the patch underutilizing the potential for a
proper inheritance hierarchy and instead relying on a staggering amounts
of copy-pasting for implementation.
I am attaching a quick untested patch illustrating how the
implementation should look like.
Also you have some leftovers from previous revision, please fix them:
Pylint is running, please wait ...
* Module ipatests.test_integration.test_trust
ipatests/test_integration/test_trust.py:236: [E1101(no-member),
TestExternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
ipatests/test_integration/test_trust.py:243:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_establish_trust] Unexpected keyword
argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:279:
[E1123(unexpected-keyword-arg),
TestExternalTrustWithSubdomain.test_remove_nonposix_trust] Unexpected
keyword argument 'subdomain' in function call)
ipatests/test_integration/test_trust.py:330: [E1101(no-member),
TestNonexternalTrustWithSubdomain.configure_dns_and_time] Module
'ipatests.test_integration.tasks' has no 'sync_time_hostname' member)
Makefile:137: recipe for target 'lint' failed
make: *** [lint] Error 1
sending incremental file list
(this is before I started meddling with it)
--
Martin^3 Babinsky
From f9849e9667e595d67ffd631f312106ae4278c135 Mon Sep 17 00:00:00 2001
From: Martin
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 06/30/2016 05:01 PM, Martin Babinsky wrote:
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable me to
reuse existing function without copy-pasting them for one variable
change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing magic
inside. The same goes with `remove trust_with_ad`. You may want to fix
the calls to them in existing code so that the domain is passed
instead of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it even
needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the
base test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain =
'.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
# no need to re-implement the function just to get to the child
AD DC hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
Hi,
thanks for review, fixed patch attached.
Lenka
From ae860f97d701b296f52de1eb0c84dd43d1429f76 Mon Sep 17 00:00:00 2001
From: Lenka Doudova
Date: Thu, 30 Jun 2016 12:23:02 +0200
Subject: [PATCH] Tests: External trust
Provides basic coverage for external trust feature.
Test cases:
1. verify an external trust with AD subdomain can be established
- verify only one trustdomain is listed
- verify subdomain users are resolvable
- verify trust can be deleted
2. verify non-external trust with AD subdomain cannot be established
3. verify an external trust with AD forest root domain can be established
- verify that even if AD subdomain is specified, it is not associated with the trust
- verify trust can be deleted
https://fedorahosted.org/freeipa/ticket/5743
---
ipatests/test_integration/tasks.py | 11 +-
ipatests/test_integration/test_trust.py | 218
2 files changed, 197 insertions(+), 32 deletions(-)
diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 5be7cdae3ac777bbf0fc52e6c511969e9fabcd72..48ef6f1963e2405d7db9a18799e9796b7f10bfe5 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -443,7 +443,7 @@ def configure_dns_for_trust(master, ad):
pass
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
"""
Establishes trust with Active Directory. Trust type is detected depending
on the presence of SfU
Re: [Freeipa-devel] [PATCH 0025][Tests] RFE: External trust
On 06/30/2016 03:47 PM, Lenka Doudova wrote:
Hi,
attaching patch with some basic coverage for external trust feature. Bit
more detailed info in commit message.
Since the feature requires me to run commands previously used only for
forest root domains even for subdomains, I made some changes in
ipatests/test_integration/tasks.py file, so that it would enable me to
reuse existing function without copy-pasting them for one variable change.
Lenka
Hi Lenka,
I have a few comments:
1.)
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad, extra_args=(), subdomain=False):
This modification seems extremely kludgy to me.
I would rather change the function signature to:
-def establish_trust_with_ad(master, ad, extra_args=()):
+def establish_trust_with_ad(master, ad_domain, extra_args=()):
and pass the domain name itself to the function instead of doing magic
inside. The same goes with `remove trust_with_ad`. You may want to fix
the calls to them in existing code so that the domain is passed instead
of the whole trust object.
2.)
+def sync_time_hostname(host, srv_hostname):
+"""
+Syncs time with the remote server given by its hostname.
+Please note that this function leaves ntpd stopped.
+"""
+host.run_command(['systemctl', 'stop', 'ntpd'])
+host.run_command(['ntpdate', srv_hostname])
+
+
this looks like a duplicate of the function above it, why is it even needed?
3.)
+class TestExternalTrustWithSubdomain(ADTrustBase):
+"""
+Test establishing external trust with subdomain
+"""
+
+@classmethod
+def install(cls, mh):
+super(ADTrustBase, cls).install(mh)
+cls.ad = cls.ad_domains[0].ads[0]
+cls.install_adtrust()
+cls.check_sid_generation()
+
+# Determine whether the subdomain AD is available
+try:
+child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+cls.ad_subdomain = None
+
+cls.configure_dns_and_time()
Please use proper OOP practices when overriding the behavior of the base
test class.
For starters you could rewrite the install method like this:
+@classmethod
+def install(cls, mh):
+# Determine whether the subdomain AD is available
+try:
+cls.child_ad = cls.host_by_role(cls.optional_extra_roles[0])
+cls.ad_subdomain = '.'.join(child_ad.hostname.split('.')[1:])
+cls.ad_subdomain_hostname = child_ad.hostname
+except LookupError:
+raise nose.SkipTest("AFAIK this will skip the whole test
class")
+super(ADTrustBase, cls).install(mh)
with child_ad stored as class attribute, you can override
`configure_dns_and_time`:
+def configure_dns_and_time(cls):
+tasks.configure_dns_for_trust(cls.master, cls.ad_subdomain)
# no need to re-implement the function just to get to the child AD DC
hostname now
+tasks.sync_time(cls.master, cls.child_ad.hostname)
You can then use this class as an intermediary in the hierarchy and
inherit the other external/non-external trust test classes from it,
since most setup method in them are just copy-pasted from this one.
--
Martin^3 Babinsky
--
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 0025][Tests] RFE: External trust
On 06/30/2016 04:12 PM, Oleg Fayans wrote: Hi Lenka, The changes in test_trust.py are fine. As for tasks.py: 1. I'd rename sync_time_hostname to just sync_time and There's already one sync_time function with different contents, it seemed nicer to create a new function than adding some "switch" into the old one, making it three time longer. 2. I would start ntpd again in the same method: it's no good to keep this thing in mind each time you call it. Leaving ntpd stopped is "legacy" from original function, and as I am not sure which purpose it serves, I decided to leave it as it is. Besides, I would split the changes into 2 patches: one for tasks.py and other for test_trust.py No problem there, + adding ticket info into commit message, as I seem to have forgot again. On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka -- 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 0025][Tests] RFE: External trust
Hi Lenka, The changes in test_trust.py are fine. As for tasks.py: 1. I'd rename sync_time_hostname to just sync_time and 2. I would start ntpd again in the same method: it's no good to keep this thing in mind each time you call it. Besides, I would split the changes into 2 patches: one for tasks.py and other for test_trust.py On 06/30/2016 03:47 PM, Lenka Doudova wrote: > Hi, > > attaching patch with some basic coverage for external trust feature. Bit > more detailed info in commit message. > > Since the feature requires me to run commands previously used only for > forest root domains even for subdomains, I made some changes in > ipatests/test_integration/tasks.py file, so that it would enable me to > reuse existing function without copy-pasting them for one variable change. > > > Lenka > > > -- Oleg Fayans Quality Engineer FreeIPA team RedHat. -- 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 0025][Tests] RFE: External trust
On 06/30/2016 04:12 PM, Oleg Fayans wrote: Hi Lenka, The changes in test_trust.py are fine. As for tasks.py: 1. I'd rename sync_time_hostname to just sync_time and 2. I would start ntpd again in the same method: it's no good to keep this thing in mind each time you call it. If you start ntpd again you may de-sync AD DC and IPA master again when there is substantial time difference between them (as is usually the case). It is best to left ntpd stopped and even disable it afterwards. Besides, I would split the changes into 2 patches: one for tasks.py and other for test_trust.py On 06/30/2016 03:47 PM, Lenka Doudova wrote: Hi, attaching patch with some basic coverage for external trust feature. Bit more detailed info in commit message. Since the feature requires me to run commands previously used only for forest root domains even for subdomains, I made some changes in ipatests/test_integration/tasks.py file, so that it would enable me to reuse existing function without copy-pasting them for one variable change. Lenka -- Martin^3 Babinsky -- 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
