[Freeipa-devel] [PATCH 0025][Tests] RFE: External trust

[Lenka Doudova]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ldoudova-0025.2-Tests-External-trust.patch
Type: text/x-patch
Size: 16957 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160701/274d5c68/attachment.bin>