[Freeipa-devel] [PATCH 0121] ipatests: Add support for hosts referenced by a keyword

[Tomas Babej]

On 10/29/2013 01:00 PM, Petr Viktorin wrote:
> On 10/24/2013 12:20 PM, Tomas Babej wrote:
>> On 10/22/2013 10:44 AM, Petr Viktorin wrote:
>>> On 10/22/2013 10:09 AM, Tomas Babej wrote:
>>>> On 10/22/2013 09:54 AM, Petr Viktorin wrote:
>>>>> On 10/22/2013 09:20 AM, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Adds support for host definition by a environment variables of the
>>>>>> following form:
>>>>>>
>>>>>> KEYWORDHOST__envX, where X is the number of the environment
>>>>>> for which host referenced by a keyword should be defined.
>>>>>>
>>>>>> You can also optionally use KEYWORDHOST__IP_envX to define
>>>>>> the IP address directly, otherwise the framework will try to resolve
>>>>>> the hostname.
>>>>>>
>>>>>> Adds a required_keyword_hosts attribute to the IntegrationTest 
>>>>>> class,
>>>>>> which can test developer use to specify the keyword hosts that this
>>>>>> particular test requires. If not all required keyword hosts are
>>>>>> available, the test will be skipped.
>>>>>>
>>>>>> All keyword hosts are accessible to the IntegrationTests via the
>>>>>> keyword_hosts attribute, which contains a dictionary keyed by the
>>>>>> keywords.
>>>>>>
>>>>>
>>>>> Why is this necessary?
>>>>> What's wrong with just extending the current scheme with more roles?
>>>>>
>>>>>
>>>>
>>>> The need for keyword hosts arised with the implementation of the 
>>>> legacy
>>>> client test suite.
>>>>
>>>> As each of these tests needs a precise type (pre-defined and
>>>> pre-configured) of VM, and not a generic client, you need to restrict
>>>> the test case to specific host type.
>>>>
>>>> One test might be restricted to RHEL 5.10 with nss-pam-ldapd, 
>>>> another to
>>>> FreeBSD, next one to CentOS with nss-pam-ldapd, next to CentOS with 
>>>> old
>>>> version of SSSD...
>>>>
>>>> Each testcase that needs a new type of preconfigured host, we would 
>>>> need
>>>> to introduce a new role, which would need to be integrated in the
>>>> framework. In such implementation, we would lose loose coupling 
>>>> between
>>>> the test framework and the test themselves, and make them less
>>>> pluggable.
>>>
>>> The framework itself (excluding the configuration part) should be able
>>> to handle this nicely using the existing role mechanism. It's true
>>> that in some places it's currently hard-wired to just a few roles, but
>>> supporting completely custom roles shouldn't be a problem.
>>> I'd prefer if this system was used, rather than basically adding a
>>> second role system, which we'd have to support even if we get rid of
>>> the current config part.
>>>
>>> The envvar-based configuration is not as flexible, but I'd rather make
>>> this part somewhat unpleasant than making the framework complex. We
>>> plan to move to a simpler configuration method anyway.
>>> That said, you can basically keep the variable name scheme you use in
>>> this patch; just maybe use TESTHOST rather than KEYWORDHOST.
>>>
>>
>> I rewired the patch to use the current role system.
>>
>> Please look if you have any additional issues.
>>
>
> I only have two naming nitpicks.
> - The roles aren't really "dynamic"; eventually all the "dynamicness" 
> should be specific just to the envvar configuration system, and a few 
> shortcuts like `replicas` for `host_by_role('replica')`. I think 
> "extra" would be a better term.
> - The environment variable names could be more descriptive. They store 
> a hostname, not a role, so instead of $ROLE_<keyword>_envX I'd prefer 
> $TESTHOST_<role>_envX
>
> Other than that the changes look great!
>

Thanks, updated patch attached.

-- 
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-0121-3-ipatests-Add-support-for-extra-roles-referenced-by-a.patch
Type: text/x-patch
Size: 17125 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131030/ef4b6452/attachment.bin>