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

[Petr Viktorin]

On 10/30/2013 03:57 PM, Tomas Babej wrote:
> 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.
>

ACK, pushed to
master: b1bffb5ecad0fdaa2f560efd2b75c76bedc4423c
ipa-3-3: 960c6bf301fe3a00205a895acabe47bac5ac9349


-- 
Petr³