On Thu, Sep 03, 2015 at 02:29:16PM +0200, Michal Židek wrote:
On 09/03/2015 09:58 AM, Lukas Slebodnik wrote:
>On (03/09/15 09:35), Jakub Hrozek wrote:
>>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 ?
>The conflict might me caused by missing integration tests in 1.12
>
>LS
Indeed it was the tests.
Patch for 1.12 is attached.
Michal
CI had some issues, but I don't think those are related to your patch:
http://sssd-ci.duckdns.org/logs/job/26/52/summary.html
ACK for sssd-1-12