Patch 0001 (sgallagh): Convert the existing options to the new names. Patch 0002 (jhrozek): Update the manpages to reflect the new names. Patch 0003 (jhrozek+sgallagh): Create a python script to convert existing config files to the new format and run it automatically during %post in the sssd.spec
Jakub has reviewed patch 0001 and I have reviewed Patch 0002 and 0003 (though I made some additional fixes to his version of 0003). I'm looking for one additional reviewer in order to push these.
Hi,
here are my comments:
On Thu, Sep 24, 2009 at 09:32:27PM -0400, Stephen Gallagher wrote:
Patch 0001 (sgallagh): Convert the existing options to the new names.
--- a/server/providers/krb5/krb5_auth.c +++ b/server/providers/krb5/krb5_auth.c @@ -895,7 +895,7 @@ int sssm_krb5_auth_init(struct be_ctx *bectx, ctx->realm = value;
ret = confdb_get_string(bectx->cdb, ctx, bectx->conf_path, - CONFDB_KRB5_REALM, "/tmp", &value); + CONFDB_KRB5_CCACHEDIR, "/tmp", &value); if (ret != EOK) goto fail; ret = lstat(value, &stat_buf); if (ret != EOK) {
Patch 0002 (jhrozek): Update the manpages to reflect the new names.
- there is too much whitespace at line 771 of the patch - in the example the sssd-krb5 the '[domains] section' is referred to and the example uses [domains/FOO] instead of [domain/FOO]. Maybe it should be underlined that this example is incomplete (missing id_proiver)
Patch 0003 (jhrozek+sgallagh): Create a python script to convert existing config files to the new format and run it automatically during %post in the sssd.spec
- is it planned to use upgrade_config.py for future upgrade, too, or is it planned to have separate tools for each upgrade. In the latter case I would call to something like upgrade_config_v1_v2.py - when using '-f /somewhere/my.conf' the backup is created in /somewhere but the new config is written to /etc/sss/sssd.conf. My expectation was that the conversion would be done in-place. If you prefer it this way '--help' should say that the default output file is /etc/sss/sssd.conf
- upgrade_config.py has problems with '%' used in the krb5_ccname_template:
ERROR: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX' Traceback (most recent call last): File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 327, in main config.upgrade_v2(options.outfile, options.backup) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 274, in upgrade_v2 self._migrate_domains() File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 210, in _migrate_domains self._migrate_domain(domain) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 200, in _migrate_domain self._migrate_kw(new_domsec, old_domsec, krb5_kw) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 101, in _migrate_kw self._migrate_if_exists(to_section, new, from_section, old) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 93, in _migrate_if_exists self._config.get(from_section, from_option)) File "/usr/lib/python2.6/ConfigParser.py", line 545, in get return self._interpolate(section, option, value, d) File "/usr/lib/python2.6/ConfigParser.py", line 613, in _interpolate self._interpolate_some(option, L, rawval, section, vars, 1) File "/usr/lib/python2.6/ConfigParser.py", line 655, in _interpolate_some "'%%' must be followed by '%%' or '(', found: %r" % (rest,)) InterpolationSyntaxError: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX'
bye, Sumit
Jakub has reviewed patch 0001 and I have reviewed Patch 0002 and 0003 (though I made some additional fixes to his version of 0003). I'm looking for one additional reviewer in order to push these.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2009 10:48 AM, Sumit Bose wrote:
Hi,
here are my comments:
On Thu, Sep 24, 2009 at 09:32:27PM -0400, Stephen Gallagher wrote:
Patch 0001 (sgallagh): Convert the existing options to the new names.
--- a/server/providers/krb5/krb5_auth.c +++ b/server/providers/krb5/krb5_auth.c @@ -895,7 +895,7 @@ int sssm_krb5_auth_init(struct be_ctx *bectx, ctx->realm = value;
ret = confdb_get_string(bectx->cdb, ctx, bectx->conf_path,
CONFDB_KRB5_REALM, "/tmp", &value);
CONFDB_KRB5_CCACHEDIR, "/tmp", &value);
Good catch! Fixed
Patch 0002 (jhrozek): Update the manpages to reflect the new names.
- there is too much whitespace at line 771 of the patch
Fixed
- in the example the sssd-krb5 the '[domains] section' is referred to and the example uses [domains/FOO] instead of [domain/FOO]. Maybe it should be underlined that this example is incomplete (missing id_proiver)
OK, noted that the example does not show any id_provider, fixed the [domains] stuff
Patch 0003 (jhrozek+sgallagh): Create a python script to convert existing config files to the new format and run it automatically during %post in the sssd.spec
- is it planned to use upgrade_config.py for future upgrade, too, or is it planned to have separate tools for each upgrade. In the latter case I would call to something like upgrade_config_v1_v2.py
Probably true, renamed
- when using '-f /somewhere/my.conf' the backup is created in /somewhere but the new config is written to /etc/sss/sssd.conf. My expectation was that the conversion would be done in-place. If you prefer it this way '--help' should say that the default output file is /etc/sss/sssd.conf
Fixed, thanks
- upgrade_config.py has problems with '%' used in the krb5_ccname_template:
ERROR: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX' Traceback (most recent call last): File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 327, in main config.upgrade_v2(options.outfile, options.backup) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 274, in upgrade_v2 self._migrate_domains() File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 210, in _migrate_domains self._migrate_domain(domain) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 200, in _migrate_domain self._migrate_kw(new_domsec, old_domsec, krb5_kw) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 101, in _migrate_kw self._migrate_if_exists(to_section, new, from_section, old) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 93, in _migrate_if_exists self._config.get(from_section, from_option)) File "/usr/lib/python2.6/ConfigParser.py", line 545, in get return self._interpolate(section, option, value, d) File "/usr/lib/python2.6/ConfigParser.py", line 613, in _interpolate self._interpolate_some(option, L, rawval, section, vars, 1) File "/usr/lib/python2.6/ConfigParser.py", line 655, in _interpolate_some "'%%' must be followed by '%%' or '(', found: %r" % (rest,)) InterpolationSyntaxError: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX'
I switched to using RawConfigParser that does not perform this interpolation - we don't use it anyway and really want raw data.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Nack. See below
On 09/25/2009 06:01 AM, Jakub Hrozek wrote:
On 09/25/2009 10:48 AM, Sumit Bose wrote:
Hi,
here are my comments:
On Thu, Sep 24, 2009 at 09:32:27PM -0400, Stephen Gallagher wrote:
Patch 0001 (sgallagh): Convert the existing options to the new names.
--- a/server/providers/krb5/krb5_auth.c +++ b/server/providers/krb5/krb5_auth.c @@ -895,7 +895,7 @@ int sssm_krb5_auth_init(struct be_ctx *bectx, ctx->realm = value;
ret = confdb_get_string(bectx->cdb, ctx, bectx->conf_path,
CONFDB_KRB5_REALM, "/tmp", &value);
CONFDB_KRB5_CCACHEDIR, "/tmp", &value);Good catch! Fixed
Patch 0002 (jhrozek): Update the manpages to reflect the new names.
- there is too much whitespace at line 771 of the patch
Fixed
- in the example the sssd-krb5 the '[domains] section' is referred to and the example uses [domains/FOO] instead of [domain/FOO]. Maybe it should be underlined that this example is incomplete (missing id_proiver)
OK, noted that the example does not show any id_provider, fixed the [domains] stuff
Patch 0003 (jhrozek+sgallagh): Create a python script to convert existing config files to the new format and run it automatically during %post in the sssd.spec
- is it planned to use upgrade_config.py for future upgrade, too, or is it planned to have separate tools for each upgrade. In the latter case I would call to something like upgrade_config_v1_v2.py
Probably true, renamed
Actually, I would prefer that we use this tool for potential future upgrades (that I hope never to have...). It would be better for it to detect which version is currently in place and upgrade it from 1 to 2, then 2 to 3, etc. internally and then write out the current version.
This way we don't need to change the spec file again for that, as well.
- when using '-f /somewhere/my.conf' the backup is created in /somewhere but the new config is written to /etc/sss/sssd.conf. My expectation was that the conversion would be done in-place. If you prefer it this way '--help' should say that the default output file is /etc/sss/sssd.conf
Fixed, thanks
Uh, this doesn't look right to me: + if options.filename and not options.outfile: + options.outfile = options.filename + if not options.outfile: + options.outfile = '/etc/sssd/sssd.conf'
I think that second if needs to be elif, otherwise you're always going to override setting options.outfile = options.filename
- upgrade_config.py has problems with '%' used in the krb5_ccname_template:
ERROR: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX' Traceback (most recent call last): File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 327, in main config.upgrade_v2(options.outfile, options.backup) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 274, in upgrade_v2 self._migrate_domains() File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 210, in _migrate_domains self._migrate_domain(domain) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 200, in _migrate_domain self._migrate_kw(new_domsec, old_domsec, krb5_kw) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 101, in _migrate_kw self._migrate_if_exists(to_section, new, from_section, old) File "/tmp/sssd_test/libexec/sssd/upgrade_config.py", line 93, in _migrate_if_exists self._config.get(from_section, from_option)) File "/usr/lib/python2.6/ConfigParser.py", line 545, in get return self._interpolate(section, option, value, d) File "/usr/lib/python2.6/ConfigParser.py", line 613, in _interpolate self._interpolate_some(option, L, rawval, section, vars, 1) File "/usr/lib/python2.6/ConfigParser.py", line 655, in _interpolate_some "'%%' must be followed by '%%' or '(', found: %r" % (rest,)) InterpolationSyntaxError: '%' must be followed by '%' or '(', found: '%h/krb5cc_%u_%U_%p_%r_%P_%%_XXXXXX'
I switched to using RawConfigParser that does not perform this interpolation - we don't use it anyway and really want raw data.
Jakub
- ------------------------------------------------------------------------
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2009 12:22 PM, Stephen Gallagher wrote:
Actually, I would prefer that we use this tool for potential future upgrades (that I hope never to have...). It would be better for it to detect which version is currently in place and upgrade it from 1 to 2, then 2 to 3, etc. internally and then write out the current version.
This way we don't need to change the spec file again for that, as well.
OK
- when using '-f /somewhere/my.conf' the backup is created in /somewhere but the new config is written to /etc/sss/sssd.conf. My expectation was that the conversion would be done in-place. If you prefer it this way '--help' should say that the default output file is /etc/sss/sssd.conf
Fixed, thanks
Uh, this doesn't look right to me:
- if options.filename and not options.outfile:
options.outfile = options.filename- if not options.outfile:
options.outfile = '/etc/sssd/sssd.conf'I think that second if needs to be elif, otherwise you're always going to override setting options.outfile = options.filename
D'oh..
the logic is fixed (note the default options are /etc/sssd/sssd.conf and None respectively)
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2009 06:53 AM, Jakub Hrozek wrote:
On 09/25/2009 12:22 PM, Stephen Gallagher wrote:
Actually, I would prefer that we use this tool for potential future upgrades (that I hope never to have...). It would be better for it to detect which version is currently in place and upgrade it from 1 to 2, then 2 to 3, etc. internally and then write out the current version.
This way we don't need to change the spec file again for that, as well.
OK
- when using '-f /somewhere/my.conf' the backup is created in /somewhere but the new config is written to /etc/sss/sssd.conf. My expectation was that the conversion would be done in-place. If you prefer it this way '--help' should say that the default output file is /etc/sss/sssd.conf
Fixed, thanks
Uh, this doesn't look right to me:
- if options.filename and not options.outfile:
options.outfile = options.filename- if not options.outfile:
options.outfile = '/etc/sssd/sssd.conf'I think that second if needs to be elif, otherwise you're always going to override setting options.outfile = options.filename
D'oh..
the logic is fixed (note the default options are /etc/sssd/sssd.conf and None respectively)
Jakub
Ack from me.
- ------------------------------------------------------------------------
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
On Fri, Sep 25, 2009 at 06:58:38AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2009 06:53 AM, Jakub Hrozek wrote:
On 09/25/2009 12:22 PM, Stephen Gallagher wrote:
Actually, I would prefer that we use this tool for potential future upgrades (that I hope never to have...). It would be better for it to detect which version is currently in place and upgrade it from 1 to 2, then 2 to 3, etc. internally and then write out the current version.
This way we don't need to change the spec file again for that, as well.
OK
> - when using '-f /somewhere/my.conf' the backup is created in /somewhere > but the new config is written to /etc/sss/sssd.conf. My expectation was > that the conversion would be done in-place. If you prefer it this way > '--help' should say that the default output file is > /etc/sss/sssd.conf
Fixed, thanks
Uh, this doesn't look right to me:
- if options.filename and not options.outfile:
options.outfile = options.filename- if not options.outfile:
options.outfile = '/etc/sssd/sssd.conf'I think that second if needs to be elif, otherwise you're always going to override setting options.outfile = options.filename
D'oh..
the logic is fixed (note the default options are /etc/sssd/sssd.conf and None respectively)
Jakub
Ack from me.
ACK from me too
bye, Sumit
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2009 07:14 AM, Sumit Bose wrote:
On Fri, Sep 25, 2009 at 06:58:38AM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/25/2009 06:53 AM, Jakub Hrozek wrote:
On 09/25/2009 12:22 PM, Stephen Gallagher wrote:
Actually, I would prefer that we use this tool for potential future upgrades (that I hope never to have...). It would be better for it to detect which version is currently in place and upgrade it from 1 to 2, then 2 to 3, etc. internally and then write out the current version.
This way we don't need to change the spec file again for that, as well.
OK
>> - when using '-f /somewhere/my.conf' the backup is created in /somewhere >> but the new config is written to /etc/sss/sssd.conf. My expectation was >> that the conversion would be done in-place. If you prefer it this way >> '--help' should say that the default output file is >> /etc/sss/sssd.conf
Fixed, thanks
Uh, this doesn't look right to me:
- if options.filename and not options.outfile:
options.outfile = options.filename- if not options.outfile:
options.outfile = '/etc/sssd/sssd.conf'I think that second if needs to be elif, otherwise you're always going to override setting options.outfile = options.filename
D'oh..
the logic is fixed (note the default options are /etc/sssd/sssd.conf and None respectively)
Jakub
Ack from me.
ACK from me too
bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
All three patches pushed to master
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Stephen Gallagher -
Stephen Gallagher -
Sumit Bose