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

[Lenka Doudova]

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