[Freeipa-devel] [PATCHES 100-106] Initial implementation of AD integration tests

[Tomas Babej]

On 10/24/2013 01:29 PM, Petr Viktorin wrote:
> On 10/22/2013 02:24 PM, Tomas Babej wrote:
>> On 10/22/2013 02:15 PM, Tomas Babej wrote:
>>> On 10/22/2013 12:27 PM, Tomas Babej wrote:
>>>> On 10/22/2013 10:37 AM, Petr Viktorin wrote:
>>>>> Replying to one part only:
>>>>>
>>>>> On 10/21/2013 04:50 PM, Tomas Babej wrote:
>>>>>> On 10/16/2013 03:44 PM, Petr Viktorin wrote:
>>>>>>>
>>>>>>> I still think it would be simpler if IPA and AD domains shared the
>>>>>>> numbering namespace (users would need to define $AD_env2; if 
>>>>>>> they had
>>>>>>> $MASTER_env1 and $AD_env1 they would be in the same Domain).
>>>>>>>
>>>>>>> But if we use _env1 for both the AD and the IPA domain, they 
>>>>>>> need to
>>>>>>> be separated in Domain.from_env. With patch 0101 both 
>>>>>>> MASTER_env1 and
>>>>>>> AD_env1 will be in both domains[0] and ad_domains[0].
>>>>>>
>>>>>> I would rather not join IPA and AD domains as they even cannot be
>>>>>> in the
>>>>>> same domain, as the service records would clash. So these will
>>>>>> always be separate / sub / super domain relationship.
>>>>>
>>>>> You're right that they should never share the same domain. But you
>>>>> should never say never, especially in testing -- what if we'll want
>>>>> to, in the future, test that the records *do* clash, or that IPA
>>>>> refuses to install in an AD domain?
>>>>>
>>>>
>>>> You could still set AD_env1 and MASTER_env1 to have the same domain.
>>>>
>>>>> Another problem is that they are now separate namespaces. In all
>>>>> code that deals with domains you have to deal separately with the
>>>>> list of AD domains and separately with IPA domains. This makes every
>>>>> piece of code that doesn't care much about what type of domain it's
>>>>> dealing with (configuration, listing, possible automation scripts
>>>>> for turning on the VMs, etc.) more complicated.
>>>>> Also, in this scheme, adding a new type of domain would be quite
>>>>> hard, especially after more code is written with this split in mind.
>>>>>
>>>>> Do keep the domain type, though. tl;dr I'd really prefer "domain 1
>>>>> (IPA); domain 2 (AD)" rather than "IPA domain 1; AD domain 1".
>>>>
>>>> This will, however, require filtering the domains depending on the
>>>> fact whether they contain AD or not. If a testcase wants to access an
>>>> AD domain, it will still need to loop over the list of domains to see
>>>> which ones are of AD type.
>>>>
>>>> Any code that does not care what domain type it's dealing with, can
>>>> easily access all the domains by chaining the respective iterables.
>>>> We could have a wrapper in the Config class for that, along the lines
>>>> of get_all_domains().
>>>>
>>>> So what I see here is that we're trading one complexity over another.
>>>>
>>>> I think we can agree on your approach since it hides the complexity
>>>> from the user, especially in the ipa-test-config, which I admit is
>>>> getting rather ugly, as we need to introduce new option there and
>>>> that causes splitting.
>>>>
>>>>>
>>>>> If needed we can have a special check that would reject IPA masters
>>>>> in AD domains and vice versa, if that really turns out to be 
>>>>> necessary.
>>>>>
>>>>
>>>> With this check we should be fine.
>>>>
>>>>>> As we already pass ad_domain flag to Domain.from_env, I did
>>>>>> incorporate
>>>>>> code that joins the machines to the domain depending on the their
>>>>>> role.
>>>>>> Is that a viable solution for you?
>>>>>
>>>>> Sorry. I think this design is less sustainable than having a shared
>>>>> namespace for the domains.
>>>>>
>>>>>
>>>> I'll send revised patchset soon.
>>>>
>>>>
>>> Updated patchset attached.
>
> Patch 105:
> Typo: 'Sets DNS ib given host for trust with the given AD
>                 ^^
> Should be "in", right? I'll fix it before pushing. c
>
> Otherwise, these are good to go!
>
>
> Patch 106:
>
> In ADTrustBase, it looks like if test_install_adtrust or 
> test_configure_dns_and_time fail, it doesn't make much sense to run 
> the other tests.
> If that's the case they can go in an install() classmethod. Same with 
> test_establish_trust* in the subclasses.
>
I made them part of the install() classmethod.

As for the test_estabilish_trust, I would have to still override that in 
each class that uses it, since all of them use it in a slightly 
different way.


> Also, there's a typo in test_estabilish_trusts several times.
>                              -----^----
>

Typo fixed. Also attaching a patch that fixes the same type in the other 
parts of the codebase.

-- 
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0122-trusts-Fix-typo-in-error-message-for-realm-domain-mi.patch
Type: text/x-patch
Size: 1313 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131024/f70d95ef/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0106-10-ipatests-Add-AD-integration-test-case.patch
Type: text/x-patch
Size: 7880 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131024/f70d95ef/attachment-0001.bin>