[PATCH] CONFDB: Assume config file version 2 if missing - sssd-devel - Fedora Mailing-Lists

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH] CONFDB: Assume config file version 2 if missing

[PATCH] SYSDB: Index the...

[PATCH] SDAP: Remove unused...

Michal Židek

Tuesday, 7 July 2015 Tue, 7 Jul '15

12:45 p.m.

Hi! See the attached patch. Ticket: https://fedorahosted.org/sssd/ticket/2688 CI passed: http://sssd-ci.duckdns.org/logs/job/18/71/summary.html Thanks, Michal -- Senior Principal Intern

Attachments:

0001-CONFDB-Assume-config-file-version-2-if-missing.patch (text/x-patch — 3.8 KB)

Reply

Show replies by date

Lukas Slebodnik

Friday, 24 July Fri, 24 Jul

2:51 p.m.

On (07/07/15 19:45), Michal Židek wrote:

Hi! See the attached patch. Ticket: https://fedorahosted.org/sssd/ticket/2688 CI passed: http://sssd-ci.duckdns.org/logs/job/18/71/summary.html Thanks, Michal -- Senior Principal Intern

From c67f8df99df4f1aafa41d57a84c8d1c19fb5d610 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 --- src/confdb/confdb.h | 1 + src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- 3 files changed, 27 insertions(+), 25 deletions(-)

I think you can also remove config_file_version from sssd.conf in integration test. Or at least comment it out. So it will be proven that your patch works. LS

Reply

Michal Židek

Monday, 27 July Mon, 27 Jul

8:58 a.m.

On 07/24/2015 09:51 PM, Lukas Slebodnik wrote:

On (07/07/15 19:45), Michal Židek wrote: > Hi! > > See the attached patch. > > Ticket: > https://fedorahosted.org/sssd/ticket/2688 > > CI passed: > http://sssd-ci.duckdns.org/logs/job/18/71/summary.html > > Thanks, > Michal > > -- > Senior Principal Intern >From c67f8df99df4f1aafa41d57a84c8d1c19fb5d610 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 > --- > src/confdb/confdb.h | 1 + > src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- > src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- > 3 files changed, 27 insertions(+), 25 deletions(-) > I think you can also remove config_file_version from sssd.conf in integration test. Or at least comment it out. So it will be proven that your patch works. LS

Ok, I added patch that removes the option from sssd.conf in integration tests. Michal -- Senior Principal Intern

Reply

Lukas Slebodnik

9:05 a.m.

On (27/07/15 15:58), Michal Židek wrote:

On 07/24/2015 09:51 PM, Lukas Slebodnik wrote: >On (07/07/15 19:45), Michal Židek wrote: >>Hi! >> >>See the attached patch. >> >>Ticket: >>https://fedorahosted.org/sssd/ticket/2688 >> >>CI passed: >>http://sssd-ci.duckdns.org/logs/job/18/71/summary.html >> >>Thanks, >>Michal >> >>-- >>Senior Principal Intern > >>From c67f8df99df4f1aafa41d57a84c8d1c19fb5d610 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 >>--- >>src/confdb/confdb.h | 1 + >>src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- >>src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- >>3 files changed, 27 insertions(+), 25 deletions(-) >> >I think you can also remove config_file_version from >sssd.conf in integration test. Or at least comment it out. >So it will be proven that your patch works. > >LS Ok, I added patch that removes the option from sssd.conf in integration tests. Michal -- Senior Principal Intern

From af960ad090a06017ce3880abaee9c07bbfbd8111 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 1/2] 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 --- src/confdb/confdb.h | 1 + src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 36df6ae..d2d7ebf 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -38,6 +38,7 @@ * @{ */ +#define CONFDB_DEFAULT_CFG_FILE_VER 2 #define CONFDB_FILE "config.ldb" #define CONFDB_DEFAULT_CONFIG_FILE SSSD_CONF_DIR"/sssd.conf" #define SSSD_MIN_ID 1 diff --git a/src/confdb/confdb_setup.c b/src/confdb/confdb_setup.c index 93a1a1b..360e3a8 100644 --- a/src/confdb/confdb_setup.c +++ b/src/confdb/confdb_setup.c @@ -224,30 +224,30 @@ int confdb_init_db(const char *config_file, struct confdb_ctx *cdb) ret = sss_ini_check_config_obj(init_data); if (ret != EOK) { - /* No known version. Assumed to be version 1 */ - DEBUG(SSSDBG_FATAL_FAILURE, - "Config file is an old version. " - "Please run configuration upgrade script.\n"); - ret = EINVAL; - goto done; - } - - version = sss_ini_get_int_config_value(init_data, 1, -1, &ret); - if (ret != EOK) { - DEBUG(SSSDBG_FATAL_FAILURE, - "Config file version could not be determined\n"); - goto done; - } 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"); + goto done; + } 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; + } } /* Set up a transaction to replace the configuration */ diff --git a/src/config/SSSDConfig/sssd_upgrade_config.py b/src/config/SSSDConfig/sssd_upgrade_config.py index 282d6c4..767d06d 100644 --- a/src/config/SSSDConfig/sssd_upgrade_config.py +++ b/src/config/SSSDConfig/sssd_upgrade_config.py @@ -47,7 +47,8 @@ class SSSDConfigFile(SSSDChangeConf): def get_version(self): ver = self.get_option_index('sssd', 'config_file_version')[1] if not ver: - return 1 + # config_file_version not found -> default to version 2 + return 2 try: return int(ver['value']) except ValueError: -- 2.1.0

From 52fb274e36c104a4b7d2df932d81a7ae07c008fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com> Date: Mon, 27 Jul 2015 14:22:08 +0200 Subject: [PATCH 2/2] tests: Remove config_file_version from sssd.conf in CI Default config_file_version was changed to 2 so the explicitly setting the value is no longer necessary. This change also serves as a test to see that the new default works. Ticket: https://fedorahosted.org/sssd/ticket/2688 ---

I prefer if so small change in test is part of "feature patch" So you do not forget to backport it. LS

Reply

Michal Židek

9:12 a.m.

On 07/27/2015 04:05 PM, Lukas Slebodnik wrote:

On (27/07/15 15:58), Michal Židek wrote: > On 07/24/2015 09:51 PM, Lukas Slebodnik wrote: >> On (07/07/15 19:45), Michal Židek wrote: >>> Hi! >>> >>> See the attached patch. >>> >>> Ticket: >>> https://fedorahosted.org/sssd/ticket/2688 >>> >>> CI passed: >>> http://sssd-ci.duckdns.org/logs/job/18/71/summary.html >>> >>> Thanks, >>> Michal >>> >>> -- >>> Senior Principal Intern >> >> >From c67f8df99df4f1aafa41d57a84c8d1c19fb5d610 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 >>> --- >>> src/confdb/confdb.h | 1 + >>> src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- >>> src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- >>> 3 files changed, 27 insertions(+), 25 deletions(-) >>> >> I think you can also remove config_file_version from >> sssd.conf in integration test. Or at least comment it out. >> So it will be proven that your patch works. >> >> LS > > Ok, I added patch that removes the option from > sssd.conf in integration tests. > > Michal > > -- > Senior Principal Intern >From af960ad090a06017ce3880abaee9c07bbfbd8111 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 1/2] 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 > --- > src/confdb/confdb.h | 1 + > src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- > src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- > 3 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h > index 36df6ae..d2d7ebf 100644 > --- a/src/confdb/confdb.h > +++ b/src/confdb/confdb.h > @@ -38,6 +38,7 @@ > * @{ > */ > > +#define CONFDB_DEFAULT_CFG_FILE_VER 2 > #define CONFDB_FILE "config.ldb" > #define CONFDB_DEFAULT_CONFIG_FILE SSSD_CONF_DIR"/sssd.conf" > #define SSSD_MIN_ID 1 > diff --git a/src/confdb/confdb_setup.c b/src/confdb/confdb_setup.c > index 93a1a1b..360e3a8 100644 > --- a/src/confdb/confdb_setup.c > +++ b/src/confdb/confdb_setup.c > @@ -224,30 +224,30 @@ int confdb_init_db(const char *config_file, struct confdb_ctx *cdb) > > ret = sss_ini_check_config_obj(init_data); > if (ret != EOK) { > - /* No known version. Assumed to be version 1 */ > - DEBUG(SSSDBG_FATAL_FAILURE, > - "Config file is an old version. " > - "Please run configuration upgrade script.\n"); > - ret = EINVAL; > - goto done; > - } > - > - version = sss_ini_get_int_config_value(init_data, 1, -1, &ret); > - if (ret != EOK) { > - DEBUG(SSSDBG_FATAL_FAILURE, > - "Config file version could not be determined\n"); > - goto done; > - } 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"); > + goto done; > + } 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; > + } > } > > /* Set up a transaction to replace the configuration */ > diff --git a/src/config/SSSDConfig/sssd_upgrade_config.py b/src/config/SSSDConfig/sssd_upgrade_config.py > index 282d6c4..767d06d 100644 > --- a/src/config/SSSDConfig/sssd_upgrade_config.py > +++ b/src/config/SSSDConfig/sssd_upgrade_config.py > @@ -47,7 +47,8 @@ class SSSDConfigFile(SSSDChangeConf): > def get_version(self): > ver = self.get_option_index('sssd', 'config_file_version')[1] > if not ver: > - return 1 > + # config_file_version not found -> default to version 2 > + return 2 > try: > return int(ver['value']) > except ValueError: > -- > 2.1.0 > >From 52fb274e36c104a4b7d2df932d81a7ae07c008fb Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek(a)redhat.com> > Date: Mon, 27 Jul 2015 14:22:08 +0200 > Subject: [PATCH 2/2] tests: Remove config_file_version from sssd.conf in CI > > Default config_file_version was changed to 2 so > the explicitly setting the value is no longer necessary. > This change also serves as a test to see that the > new default works. > > Ticket: > https://fedorahosted.org/sssd/ticket/2688 > --- I prefer if so small change in test is part of "feature patch" So you do not forget to backport it.

Squashed. Michal -- Senior Principal Intern

Reply

Lukas Slebodnik

Thursday, 30 July Thu, 30 Jul

11:08 a.m.

On (27/07/15 16:12), Michal Židek wrote:

On 07/27/2015 04:05 PM, Lukas Slebodnik wrote: >On (27/07/15 15:58), Michal Židek wrote: >>On 07/24/2015 09:51 PM, Lukas Slebodnik wrote: >>>On (07/07/15 19:45), Michal Židek wrote: >>>>Hi! >>>> >>>>See the attached patch. >>>> >>>>Ticket: >>>>https://fedorahosted.org/sssd/ticket/2688 >>>> >>>>CI passed: >>>>http://sssd-ci.duckdns.org/logs/job/18/71/summary.html >>>> >>>>Thanks, >>>>Michal >>>> >>>>-- >>>>Senior Principal Intern >>> >>>>From c67f8df99df4f1aafa41d57a84c8d1c19fb5d610 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 >>>>--- >>>>src/confdb/confdb.h | 1 + >>>>src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- >>>>src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- >>>>3 files changed, 27 insertions(+), 25 deletions(-) >>>> >>>I think you can also remove config_file_version from >>>sssd.conf in integration test. Or at least comment it out. >>>So it will be proven that your patch works. >>> >>>LS >> >>Ok, I added patch that removes the option from >>sssd.conf in integration tests. >> >>Michal >> >>-- >>Senior Principal Intern > >>From af960ad090a06017ce3880abaee9c07bbfbd8111 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 1/2] 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 >>--- >>src/confdb/confdb.h | 1 + >>src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- >>src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- >>3 files changed, 27 insertions(+), 25 deletions(-) >> >>diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h >>index 36df6ae..d2d7ebf 100644 >>--- a/src/confdb/confdb.h >>+++ b/src/confdb/confdb.h >>@@ -38,6 +38,7 @@ >> * @{ >> */ >> >>+#define CONFDB_DEFAULT_CFG_FILE_VER 2 >>#define CONFDB_FILE "config.ldb" >>#define CONFDB_DEFAULT_CONFIG_FILE SSSD_CONF_DIR"/sssd.conf" >>#define SSSD_MIN_ID 1 >>diff --git a/src/confdb/confdb_setup.c b/src/confdb/confdb_setup.c >>index 93a1a1b..360e3a8 100644 >>--- a/src/confdb/confdb_setup.c >>+++ b/src/confdb/confdb_setup.c >>@@ -224,30 +224,30 @@ int confdb_init_db(const char *config_file, struct confdb_ctx *cdb) >> >> ret = sss_ini_check_config_obj(init_data); >> if (ret != EOK) { >>- /* No known version. Assumed to be version 1 */ >>- DEBUG(SSSDBG_FATAL_FAILURE, >>- "Config file is an old version. " >>- "Please run configuration upgrade script.\n"); >>- ret = EINVAL; >>- goto done; >>- } >>- >>- version = sss_ini_get_int_config_value(init_data, 1, -1, &ret); >>- if (ret != EOK) { >>- DEBUG(SSSDBG_FATAL_FAILURE, >>- "Config file version could not be determined\n"); >>- goto done; >>- } 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 you shoudl have proper indentation.

>>+ goto done; >>+ } else if (version < CONFDB_VERSION_INT) { >>+ DEBUG(SSSDBG_FATAL_FAILURE, >>+ "Config file is an old version. " >>+ "Please run configuration upgrade script.\n");

^^^ and also here. LS

Reply

Michal Židek

Wednesday, 5 August Wed, 5 Aug

6:41 a.m.

On 07/30/2015 06:08 PM, Lukas Slebodnik wrote:

On (27/07/15 16:12), Michal Židek wrote: > On 07/27/2015 04:05 PM, Lukas Slebodnik wrote: >> On (27/07/15 15:58), Michal Židek wrote: >>> On 07/24/2015 09:51 PM, Lukas Slebodnik wrote: >>>> On (07/07/15 19:45), Michal Židek wrote: >>>>> Hi! >>>>> >>>>> See the attached patch. >>>>> >>>>> Ticket: >>>>> https://fedorahosted.org/sssd/ticket/2688 >>>>> >>>>> CI passed: >>>>> http://sssd-ci.duckdns.org/logs/job/18/71/summary.html >>>>> >>>>> Thanks, >>>>> Michal >>>>> >>>>> -- >>>>> Senior Principal Intern >>>> >>>> >From c67f8df99df4f1aafa41d57a84c8d1c19fb5d610 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 >>>>> --- >>>>> src/confdb/confdb.h | 1 + >>>>> src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- >>>>> src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- >>>>> 3 files changed, 27 insertions(+), 25 deletions(-) >>>>> >>>> I think you can also remove config_file_version from >>>> sssd.conf in integration test. Or at least comment it out. >>>> So it will be proven that your patch works. >>>> >>>> LS >>> >>> Ok, I added patch that removes the option from >>> sssd.conf in integration tests. >>> >>> Michal >>> >>> -- >>> Senior Principal Intern >> >> >From af960ad090a06017ce3880abaee9c07bbfbd8111 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 1/2] 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 >>> --- >>> src/confdb/confdb.h | 1 + >>> src/confdb/confdb_setup.c | 48 ++++++++++++++-------------- >>> src/config/SSSDConfig/sssd_upgrade_config.py | 3 +- >>> 3 files changed, 27 insertions(+), 25 deletions(-) >>> >>> diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h >>> index 36df6ae..d2d7ebf 100644 >>> --- a/src/confdb/confdb.h >>> +++ b/src/confdb/confdb.h >>> @@ -38,6 +38,7 @@ >>> * @{ >>> */ >>> >>> +#define CONFDB_DEFAULT_CFG_FILE_VER 2 >>> #define CONFDB_FILE "config.ldb" >>> #define CONFDB_DEFAULT_CONFIG_FILE SSSD_CONF_DIR"/sssd.conf" >>> #define SSSD_MIN_ID 1 >>> diff --git a/src/confdb/confdb_setup.c b/src/confdb/confdb_setup.c >>> index 93a1a1b..360e3a8 100644 >>> --- a/src/confdb/confdb_setup.c >>> +++ b/src/confdb/confdb_setup.c >>> @@ -224,30 +224,30 @@ int confdb_init_db(const char *config_file, struct confdb_ctx *cdb) >>> >>> ret = sss_ini_check_config_obj(init_data); >>> if (ret != EOK) { >>> - /* No known version. Assumed to be version 1 */ >>> - DEBUG(SSSDBG_FATAL_FAILURE, >>> - "Config file is an old version. " >>> - "Please run configuration upgrade script.\n"); >>> - ret = EINVAL; >>> - goto done; >>> - } >>> - >>> - version = sss_ini_get_int_config_value(init_data, 1, -1, &ret); >>> - if (ret != EOK) { >>> - DEBUG(SSSDBG_FATAL_FAILURE, >>> - "Config file version could not be determined\n"); >>> - goto done; >>> - } 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.

>>> + goto done; >>> + } else if (version < CONFDB_VERSION_INT) { >>> + DEBUG(SSSDBG_FATAL_FAILURE, >>> + "Config file is an old version. " >>> + "Please run configuration upgrade script.\n"); ^^^ and also here. LS

Reply

Lukas Slebodnik

Thursday, 6 August Thu, 6 Aug

6:39 a.m.

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. Please also remove config_file_version from test_memory_cache.py LS

Reply

Michal Židek

Monday, 10 August Mon, 10 Aug

10:18 a.m.

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 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 little inconveniences.

Please also remove config_file_version from test_memory_cache.py

We do not necessary need to have all tests without the option to test if the default works. But I removed it from test_memory_cache.py anyway. Michal

LS _______________________________________________ sssd-devel mailing list sssd-devel(a)lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Reply

Lukas Slebodnik

Tuesday, 11 August Tue, 11 Aug

12:47 a.m.

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

Reply

Jakub Hrozek

Tuesday, 1 September Tue, 1 Sep

3:51 a.m.

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?

Reply

Lukas Slebodnik

4 a.m.

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

Reply

Jakub Hrozek

4:11 a.m.

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

Reply

Michal Židek

5:55 a.m.

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

Reply

Lukas Slebodnik

Wednesday, 2 September Wed, 2 Sep

7:52 a.m.

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

Reply

Jakub Hrozek

Thursday, 3 September Thu, 3 Sep

2:35 a.m.

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 ?

Reply

Lukas Slebodnik

2:58 a.m.

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

Reply

Michal Židek

7:29 a.m.

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

Reply

Jakub Hrozek

Monday, 14 September Mon, 14 Sep

4:10 a.m.

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

Reply

Jakub Hrozek

4:57 a.m.

On Mon, Sep 14, 2015 at 11:10:53AM +0200, Jakub Hrozek wrote:

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

* sssd-1-12: c35eb4aa64a67d34d343d608be40d60b61fb7d11

Reply

3147

days inactive

3216

days old

sssd-devel@lists.fedorahosted.org

Manage subscription

19 comments

3 participants

tags (0)

participants (3)

Jakub Hrozek
Lukas Slebodnik
Michal Židek