[SSSD] [PATCHSET] intg: sss_override

Pavel Reichl preichl at redhat.com
Wed Oct 21 11:04:24 UTC 2015



On 10/21/2015 11:31 AM, Nikolai Kondrashov wrote:
> On 10/20/2015 05:15 PM, Pavel Reichl wrote:
>>
>>
>> On 10/20/2015 03:12 PM, Nikolai Kondrashov wrote:
>>> Hi Pavel,
>>>
>>> On 10/13/2015 12:22 AM, Pavel Reichl wrote:
>>>> Please see updated patchset.
>>>
>>> Thank you for working on this. Please see my comments below.
>>>
>>> First of all, we need the test in Makefile.am, so it is included into
>>> distribution.
>>>
>>> I'm not sure if we really need this split into three patches, to me a single
>>> patch seems better. Anyone against?
>>
>> Well, I don't like huge patches, but I probably won't cry if we decide to
>> merge them.  :-)
>
> Yeah, it'll be big, but it'll be simple - just adding a file. Simpler than
> three separate patches anyway. At least for me it would have been easier to
> review this change as a monolithic patch.

OK, lets compromise, I'll merge first 2 patches - the core tests. I'll keep the 3rd patch, which tests bugs that were already reported and fixed, as a separate patch.

>
>>> Shouldn't a global variable name be uppercase?
>> OK, might be I just replace it with
>>
>> def get_user_export_file():
>>      return "user_export_file"
>
> Erm, no, I'd rather keep the lowercase variable then :) If we don't have a
> convention for uppercase global variables, then it's fine with me.

Actually I asked IPA guys and they agree with upper case - so fine by me. I just wanted to work around the fact that there are no constants in python so I thought using function would solve this.
But I can live with USER_EXPORT_FILE.
>
>>>
>>>> +
>>>> +
>>>> + at pytest.fixture
>>>> +def sanity_rfc2307(request, ldap_conn):
>>>
>>> Does this and other function have to have "_rfc2307" in them?
>>> Do you plan tests for other schemas?
>> No, would you propose a better name? Does "set_test_user_env()" makes sense?
>
> Oh, no, this wouldn't be right. Fixtures are supposed to be named after
> particular object or state they provide, they shouldn't be actions (even
> though they're functions).
>
> I was just suggesting removing "_rfc2307" from them if you're not going to be
> running with other schemas and thus wouldn't also need e.g. "_rfc2307_bis"
> version anytime soon.
>
> Otherwise, "sanity" is OK and is supposed to mean the most basic test there
> could be to ensure running the test suite even makes sense, or such.

Thanks for explaining meaning of sanity. It was a bit mystery to me :-)

  A fixture
> named thusly would indicate it belongs to such test. Since so far it's the
> only fixture we needed, there is no sense in naming it otherwise, but given we
> add more we'll need to be more inventive.
>
>>>> +    restart_sssd()
>>>
>>> Why do we need to restart SSSD here?
>>
>> After override you have to restart SSSD to make the change available.
>
> Hmm, I didn't find anything about this in sss_override(8).
> Have found a documentation bug?

Probably yes, patch is already on the list. Thanks!




More information about the sssd-devel mailing list