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

[Martin Babinsky]

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'.

-- 
Martin^3 Babinsky