-----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);
> if (ret != EOK) goto fail;
> ret = lstat(value, &stat_buf);
> if (ret != EOK) {
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(a)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 SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkq8md4ACgkQeiVVYja6o6NFAQCgjH1vkCZUnyHOXXRYGBU9sqKA
CtIAnjN5klt2WWkAFO/Ra95X6Wt+28SG
=BypZ
-----END PGP SIGNATURE-----