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
