-----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@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-----
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
