On Tue, Sep 01, 2015 at 11:00:15AM +0200, Lukas Slebodnik wrote:
> On (01/09/15 10:51), Jakub Hrozek wrote:
>> On Tue, Aug 11, 2015 at 07:47:00AM +0200, Lukas Slebodnik wrote:
>>> On (10/08/15 17:18), Michal Židek wrote:
>>>> On 08/06/2015 01:39 PM, Lukas Slebodnik wrote:
>>>>> On (05/08/15 13:41), Michal Židek wrote:
>>>>>> On 07/30/2015 06:08 PM, Lukas Slebodnik wrote:
>>>>>>>>>> - } else if (version < CONFDB_VERSION_INT)
{
>>>>>>>>>> - DEBUG(SSSDBG_FATAL_FAILURE,
>>>>>>>>>> - "Config file is an old
version. "
>>>>>>>>>> - "Please run configuration
upgrade script.\n");
>>>>>>>>>> - ret = EINVAL;
>>>>>>>>>> - goto done;
>>>>>>>>>> - } else if (version > CONFDB_VERSION_INT)
{
>>>>>>>>>> - DEBUG(SSSDBG_FATAL_FAILURE,
>>>>>>>>>> - "Config file version is
newer than confdb\n");
>>>>>>>>>> - ret = EINVAL;
>>>>>>>>>> - goto done;
>>>>>>>>>> + /* No known version. Use default. */
>>>>>>>>>> + DEBUG(SSSDBG_CONF_SETTINGS,
>>>>>>>>>> + "Value of config_file_version
option not found. "
>>>>>>>>>> + "Assumed to be version
%d.\n", CONFDB_DEFAULT_CFG_FILE_VER);
>>>>>>>>>> + } else {
>>>>>>>>>> + version =
sss_ini_get_int_config_value(init_data,
>>>>>>>>>> +
CONFDB_DEFAULT_CFG_FILE_VER,
>>>>>>>>>> +
-1, &ret);
>>>>>>>>>> + if (ret != EOK) {
>>>>>>>>>> + DEBUG(SSSDBG_FATAL_FAILURE,
>>>>>>>>>> + "Config file version
could not be determined\n");
>>>>>>> ^^
>>>>>>> I do not prefer nested "if"s. If you decided to do
it in this way then
>>>>>>
>>>>>> Me neither, but sss_ini_get_int_config_value() has to be
>>>>>> skipped conditionally. It is just call to the function
>>>>>> plus error checking that is nested. I think it is not
>>>>>> too bad in this case.
>>>>>>
>>>>>>> you shoudl have proper indentation.
>>>>>>>
>>>>>>
>>>>>> Fixed in the new version.
>>>>>>
>>>>> It works because integration tests passed.
>>>>>
http://sssd-ci.duckdns.org/logs/job/20/68/summary.html
>>>>> But ...
>>>>>
>>>>> I tested new version with ipa-client-install
>>>>> and "config_file_version = 2" is still added to sssd.conf
>>>>> even though it is a default value.
>>>>>
>>>>> ipa-client-install uses our python API (python-sssdconfig)
>>>>> and it does not try to add this option itself.
>>>>
>>>> I often change version of SSSD with git checkout sssd<version>.
>>>> If I generate the config with realmd or ipa-client-install
>>> realmd do not use python module SSSDConfig.
>>>
>>>> with latest version then the config_version_file
>>>> would need to be added manually (and I am pretty
>>>> sure it would be after I looked into logs to see why sssd
>>>> is not starting). I know this will probably only hit
>>>> testers/developers, but I would prefer not to add unnecessary
>>> just developers and developers useually join machine
>>> to AD or IPA just once.
>>>
>>>> little inconveniences.
>>>>
>>> I do not try old sssd version very often. Just in case of bisect.
>>> and ther is nice/simple workarount for "little inconvenience"
>>>
>>> Just manually add "config_version_file = 2" to sssd.conf
>>>
>>> The main point of this ticket is to simplify sssd.conf
>>> and our python sssdconfig API should do the same.
>>> Otherwise we do not need to do such change.
>>>
>>> I still see "config_file_version = 2" in sssd.conf
>>> It was generated by python module SSSDConfig (via ipa-client-install)
>>>
>>> LS
>>
>> This thread has stalled, let's try to restart it.
>>
>> What versions are normally still being worked on by developers? I
>> suspect it's master and sssd-1-12. What about pushing the patch to
>> sssd-1-12 as well?
> I'm fine with sssd-1-12.
> The python sssdconfig need to be updated.
>
> LS
Michal, if you agree, please update the patches so that we can push them
in time for sssd-1.13.1
New patch attached. I tested ipa-client-install and the
config_file_version is no longer added.
Michal