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

[Martin Babinsky]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-desired-test-class-hierarchy.patch
Type: text/x-patch
Size: 7903 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160701/41a2bbb5/attachment.bin>