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: 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 patch attached.
Lenka
Hi Lenka,
'TestExternalTrustWithSubdomain' and 'TestExternalTrustWithRootDomain'
suites fail due to incorrectly used '--external' option in the
trust-add command. This option is Boolean, not Flag so it should be
'--external=True'.
Hi,
that must have changes since I sent the patch, it was fine before.
Anyway, fixed patch attached.
Lenka
Hi Lenka,
one of the tests keep failing on non-posix user UID/GID resolution. Is
it a bug of the test or incorrect setup of AD machines?
https://paste.fedoraproject.org/392195/68853561/
--
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
