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

[Lenka Doudova]

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