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

[Petr Viktorin]

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!

-- 
Petr³