On Wed, Sep 02, 2015 at 02:52:42PM +0200, Lukas Slebodnik wrote:
On (01/09/15 12:55), Michal Židek wrote:
>On 09/01/2015 11:11 AM, Jakub Hrozek wrote:
>>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
>
>From f61ca88d69b3a56645b7473928b0674ac4bceff8 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com>
>Date: Tue, 7 Jul 2015 15:15:32 +0200
>Subject: [PATCH] CONFDB: Assume config file version 2 if missing
>
>Default to config file version 2 if the version
>is not specified explicitly.
>
>Ticket:
>https://fedorahosted.org/sssd/ticket/2688
>---
* integration tests passed
* ipa-client generated config without config_file_version and sssd worked.
ACK
LS
* master: 175613be0cfb0890174d12d941e634d833b63dd9
Michal, can you rebase the patch for sssd-1-12 ?