Edgewall Software
Modify

Opened 9 years ago

Last modified 4 years ago

#9674 new enhancement

[PATCH] trac.ini parameterization support

Reported by: sahendrickson@… Owned by:
Priority: normal Milestone: next-major-releases
Component: general Version: 0.12
Severity: normal Keywords: config trac.ini patch
Cc: leho@…, Thijs Triemstra, osimons Branch:
Release Notes:

config: %(envname)s in a setting value expands to the basename of the environment's path

API Changes:

config: internally, the trac.ini file is parsed using a SafeConfigParser instead of a ConfigParser

Description

I'm running trac using the trac.env_parent_dir option. It would be useful to have a variable that can be expanded within an inherited trac.ini file, which indicates the subfolder for the trac instance using it. (Of course, other variables could also be conceived of).

For example, given the following layout of trac environments:

/trac
  - /env1
  - /env2
  - /env3

A common trac.ini file could be created that references a %{project} variable, which is interpreted to be env1, env2, or env3, depending on which environment is inheriting the configuration.

The following is all the instances in a common trac.ini file for which I found the variable useful (in my setup, at least):

[account-manager]
authentication_url = https://some.domain.com/trac/%{project}/login

[authz_policy]
authz_file = /var/auth/trac.%{project}.authz

[notification]
smtp_from = trac-%{project}@some.domain.com

[trac]
authz_file = /etc/apache2/auth/authz/trac.%{project}.authz
base_url = https://some.domain.com/trac/%{project}/

[project]
url = https://some.domain.com/trac/%{project}/

If it were just one field that needed customization, I'd override it in the individual trac.ini files of each environment. Or, if it were a set of fields within a common section, I could use ConfigParser's support for %variable within that section (1). But, since it's many fields across many sections (as shown above), I went ahead and looked into creating a patch.

(1) A relevant Python patch I ran across while looking for solutions: http://bugs.python.org/issue9876

Attachments (2)

t9674_trac-0.12_r10194.patch (4.6 KB ) - added by sahendrickson@… 9 years ago.
Adds support for %{project} variable to (inherited) trac.ini files
t9674-config_option_interpolate-r10640.diff (3.2 KB ) - added by osimons 8 years ago.
Patch for interpolation of Option descriptor.

Download all attachments as: .zip

Change History (32)

by sahendrickson@…, 9 years ago

Adds support for %{project} variable to (inherited) trac.ini files

comment:1 by lkraav <leho@…>, 9 years ago

Cc: leho@… added

comment:2 by Christian Boos, 9 years ago

Keywords: patch added
Milestone: 0.13
Owner: set to Christian Boos

Patch looks good, maybe we should use envname rather than project here (MultiProject).

comment:3 by Christian Boos, 9 years ago

I wonder if we shouldn't use instead the default interpolation mechanism of SafeConfigParser:

  • trac/config.py

    diff -r c761ba5951be trac/config.py
    a b  
    1414
    1515from __future__ import with_statement
    1616
    17 from ConfigParser import ConfigParser
     17from ConfigParser import SafeConfigParser
    1818from copy import deepcopy
    1919import os.path
    2020
    class Configuration(object):  
    5050    the last modification time of the configuration file, and reparses it
    5151    when the file has changed.
    5252    """
    53     def __init__(self, filename):
     53    def __init__(self, filename, params={}):
    5454        self.filename = filename
    55         self.parser = ConfigParser()
     55        self.parser = SafeConfigParser(params)
    5656        self._old_sections = {}
    5757        self.parents = []
    5858        self._lastmtime = 0
  • trac/env.py

    diff -r c761ba5951be trac/env.py
    a b class Environment(Component, ComponentMa  
    550550
    551551    def setup_config(self):
    552552        """Load the configuration file."""
    553         self.config = Configuration(os.path.join(self.path, 'conf',
    554                                                  'trac.ini'))
     553        self.config = Configuration(os.path.join(self.path, 'conf', 'trac.ini'),
     554                                    {'envname': os.path.basename(self.path)})
    555555        self.setup_log()
    556556        from trac.loader import load_components
    557557        plugins_dir = self.shared_plugins_dir

comment:4 by Christian Boos, 9 years ago

API Changes: modified (diff)
Keywords: config added
Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

Above patch applied in r10565.

comment:5 by Christian Boos, 9 years ago

Resolution: fixed
Status: closedreopened

Unforeseen side-effect: now 'envname' is considered to be present in every sections!

>>> c = RawConfigParser({'envname': 'cblap'})
>>> c.read('.../conf/trac.ini')
>>> c.options('ticket-custom')
['customer', 'envname', 'customer.options', 'customer.value']

Not what I expected from the docs, but looking at the code, it seems to be really done on purpose…

One possibility would be to use __default__envname for the key, and filter out any option starting with __default__. Thoughts?

in reply to:  5 comment:6 by Christian Boos, 9 years ago

One possibility would be to use __default__envname for the key, and filter out any option starting with __default__.

Or better yet: use '#envname' and filter out anything starting with '#'. This is shorter and can't possibly conflict with something defined in the TracIni file, as a leading '#' would introduce a comment.

comment:7 by Remy Blank, 9 years ago

Sounds very hackish… Can't we implement only the part of string interpolation that we need on top of ConfigParser?

comment:8 by Christian Boos, 9 years ago

Well, yes, we could go back using the RawConfigParser and do the expansion ourselves.

comment:9 by Thijs Triemstra, 9 years ago

Cc: Thijs Triemstra added
Summary: trac.ini parameterization support [patch][PATCH] trac.ini parameterization support

comment:10 by osimons, 8 years ago

Cc: osimons added

comment:11 by Remy Blank, 8 years ago

Do you mind if I revert [10565] temporarily, until a better solution is available? This "Envname" custom field is quite ugly…

For a better solution, shouldn't we rather use RawConfigParser and override get() and items() to add our own interpolation? If I understand correctly, we currently use ConfigParser but don't use (or discourage the use of) its interpolation functionality. Probably because interpolating with all other keys in the config file is not very intuitive.

comment:12 by Christian Boos, 8 years ago

I'd obviously prefer if you could give a try to the config-refact.patch:ticket:7378, rather than going in another direction with RawConfigParser.

comment:13 by Remy Blank, 8 years ago

Oh, right, I had forgotten about that one. Sure, I will give it a try. Can we still revert [10565] in the meantime?

comment:14 by Christian Boos, 8 years ago

Maybe not [10565] wholesale, as the changes in trunk/trac/env.py is going to stay in the new version. So just going back to self.parser = ConfigParser() (and fixing the import) should be enough.

comment:15 by Remy Blank, 8 years ago

Sure, as the patch on #7378 already has the params argument. One question: are the variables in params also expanded in inherited configuration files? In the current state, I would assume not, as they aren't passed up the parent chain. I think they should, as that's where these expansions become really useful.

comment:16 by Remy Blank, 8 years ago

Reverted the use of SafeConfigParser in [10640]. Review of the config parsing refactoring still pending.

comment:17 by Remy Blank, 8 years ago

… and actually removed the parameter expansion in [10641]. Ah, Mondays…

comment:18 by Remy Blank, 8 years ago

Funny thing, I updated teo to [10641], and the envname custom field disappeared. However, on the 0.13 demo instance, it remained visible. It turned out that a configuration change triggered writing of trac.ini while expansion was active, and this inserted an envname = trac-0.13 entry into every section. I have removed the spurious entries.

in reply to:  18 ; comment:19 by osimons, 8 years ago

Replying to rblank:

It turned out that a configuration change triggered writing of trac.ini while expansion was active, and this inserted an envname = trac-0.13 entry into every section.

This is quite important, actually. And scary, particularly when managing a lot of Trac instances… I do not feel comfortable with not storing the pre-expansion value, and can certainly see how easily they could be overwritten on any config change. I suppose this is a discussion of whether to expand when parsing the file or when accessing the option.

This is no trivial issue, and when done across all options equally (as with the parsing strategy) side-effects may also occur. There are settings in Trac and various plugins that use the ${} for fields with variable expansion like subject lines and so on. Roughly half my settings are from code outside default Trac…

I'd prefer if expansion could be enabled/disabled on option level, like if Option() and its relatives gained a new interpolate keyword argument. This could even be expanded to be interpolate=None default, with the option to pass in your own dictionay for interpolation of the option: interpolate={'__envname__': self.env.project_name, '__one__': my_var, '__two__': self.my_prop}. Really any type that knows how to be a str.

That would provide a much more flexible and general solution for Trac and developers, and without the dependency on us to add generally-useful-for-all parameters for interpolation that end up covering the very basic needs of a few options. For many of the options I depend on, the interpolate-on-read could also mean that I could just provide the default as the interpolation variable string, and have all representations be generated by plugin code on-the-fly.

As a side-effect, dynamic interpolation could perhaps also ease the transition from storing in configuration file to storing at arbitrary location such as databases. And also a base on which to solve various open ticket here and at trac-hacks.org that requests/suggests various hacks to provide dynamic configuration content.

in reply to:  19 ; comment:20 by Christian Boos, 8 years ago

Replying to osimons:

Replying to rblank:

It turned out that a configuration change triggered writing of trac.ini while expansion was active, and this inserted an envname = trac-0.13 entry into every section.

This is quite important, actually. And scary, particularly when managing a lot of Trac instances… I do not feel comfortable with not storing the pre-expansion value, and can certainly see how easily they could be overwritten on any config change.

Well, the spread of that global setting into a key within all sections is just yet another "funny" side-effect of the ConfigParser… nothing we should really worry about as we're getting away from this class and its quirks.

But you're right in saying that this raises the more general question of taking care not to write back the expanded value into the config file, something we should certainly take care about in the new system.

I suppose this is a discussion of whether to expand when parsing the file or when accessing the option.

This is an interesting point indeed. We currently cache the actual string value of an entry on first access, so allowing anything "dynamic" at a finer grain than a value valid for a whole environment would probably invalidate this simple model.

This is no trivial issue, and when done across all options equally (as with the parsing strategy) side-effects may also occur. There are settings in Trac and various plugins that use the ${} for fields with variable expansion like subject lines and so on.

Well, things like configured values for templates, which also support expansions, are a different matter than what was initially targeted in this ticket, I believe. We have such "dynamic" values in different places in the code, and each time it's a quite different expansion syntax and logic, with little hope (or even interest) to unify them, for example:

  • [notification] ticket_subject_template: a Genshi text template, with a specific ticket giving the context
  • [logging] log_format: $(key)s style expansion mainly done by the logger itself (+ a few keys specific from us); note that we could support directly the actual logging format style %(...)s once we disable automatic global expansion a la ConfigParser
  • [svn:externals] $path?rev=$rev style mapping

In all those situations, the ConfigParser style expansion just gets in the way, or at the very least makes things more complicated.

Roughly half my settings are from code outside default Trac…

I'd prefer if expansion could be enabled/disabled on option level, like if Option() and its relatives gained a new interpolate keyword argument.

I think that's a good idea. We could therefore leave the "pattern" in the _cache dictionary I mentioned above. The only problem would be that a generic access (like Section.get(key)) will give you back the pattern, not the expanded value, only access via the actual Option will expand it correctly. This is not necessarily a problem though.

This could even be expanded to be interpolate=None default, with the option to pass in your own dictionay for interpolation of the option: interpolate={'__envname__': self.env.project_name, '__one__': my_var, '__two__': self.my_prop}. Really any type that knows how to be a str.

That wouldn't work as such (no self at that point), at least you'd need a lambda of some sort. Also, most PathOption would benefit from the same set of variables (mainly the environment envname, envdir, and possibly env_parent_dir).

That would provide a much more flexible and general solution for Trac and developers, and without the dependency on us to add generally-useful-for-all parameters for interpolation that end up covering the very basic needs of a few options.

For many of the options I depend on, the interpolate-on-read could also mean that I could just provide the default as the interpolation variable string, and have all representations be generated by plugin code on-the-fly.

Possibly. Maybe you can show us a small concrete example to make this clearer?

As a side-effect, dynamic interpolation could perhaps also ease the transition from storing in configuration file to storing at arbitrary location such as databases. And also a base on which to solve various open ticket here and at trac-hacks.org that requests/suggests various hacks to provide dynamic configuration content.

Dynamic configuration content is an interesting and complex topic :-)

in reply to:  20 ; comment:21 by osimons, 8 years ago

Replying to cboos:

This is an interesting point indeed. We currently cache the actual string value of an entry on first access, so allowing anything "dynamic" at a finer grain than a value valid for a whole environment would probably invalidate this simple model.

Hmm. That sure need some testing and thinking then. But, if finer grain is valuable then disabling caching of options defined with interpolation dictionaries is necessary.

Well, things like configured values for templates, which also support expansions, are a different matter than what was initially targeted in this ticket, I believe.

Sorry, maybe I wasn't clear - my only point was that whatever interpolation-for-all-fields general strategy chosen by us, it may well intentionally or unintentionally conflict with what plugins++ may expect from its fields. We do agree here :-)

I think that's a good idea. We could therefore leave the "pattern" in the _cache dictionary I mentioned above. The only problem would be that a generic access (like Section.get(key)) will give you back the pattern, not the expanded value, only access via the actual Option will expand it correctly. This is not necessarily a problem though.

I would expect config.get('my_option') to return the expanded value from wherever it is called.

That wouldn't work as such (no self at that point), at least you'd need a lambda of some sort.

Yes, perhaps not so clearly written. It was just meant for conveying the idea. Also, there is likely no reason why we can't check the dictionary to see if the value is callable and call it and use its result? We can do interpolation any way we want.

Anyway, the idea was that the Option would store the entire dictionary and anything added to it should be available by reference later. Even when pointing at some initial self of a plugin. The link between configuration and plugin should be stable enough to survive, and will be redone whenever config or both reloads. Or, do we have issues with order of loading? Hmm. It would need some testing too…

Possibly. Maybe you can show us a small concrete example to make this clearer?

Something like this illustration perhaps:

   ...
   item_path = Option(...,
                  default="${__path__}",
                  ...,
                  interpolate={'__path__': self.get_item_path}) 

Think for instance the migration of repository_dir and repository_type settings where transitional defaults could just be an interpolated variable that gets looked up from code. That would allow any plugin that still reads the old config to work correctly, but the actual value is really supplied from database.

Dynamic configuration content is an interesting and complex topic :-)

Indeed.

BTW, for a large part the ideas from the original patch from sahendrickson are in line with what I suggest - the only real change is that the interpolation dictionary would be supplied by option definition instead of shared by all options. Should be doable to update the patch. I'll see if I can find time later tonight.

by osimons, 8 years ago

Patch for interpolation of Option descriptor.

in reply to:  21 comment:22 by osimons, 8 years ago

Replying to osimons:

I would expect config.get('my_option') to return the expanded value from wherever it is called.

Expect it, but not easily done… I see what you said now Christian, and after actually looking at the code again I see that adding it for descriptors is easy. Attached patch circumvents all issues of caching and config reading + writing.

Any effort to support config.get() quickly started to look quite ugly, but I'm open for suggestions. It would be nice, but not at any cost. Calling option via component is at least a natural trade-off.

BTW, I just kept the substitution code from original patch. It is more for proof-of-concept than endorsement of any particular syntax for substitution. Need to decide that though.

Last edited 8 years ago by osimons (previous) (diff)

comment:23 by Remy Blank, 8 years ago

At least this:

for m in reversed( 
        [m for m in re.finditer(r"%{(\w+)}", value)]):

could be replaced with:

for m in reversed(re.finditer(r"%{(\w+)}", value)):

But that's an implementation detail :)

in reply to:  23 comment:24 by osimons, 8 years ago

Replying to rblank:

…could be replaced with:

Actually, that did not work: TypeError: argument to reversed() must be a sequence

for m in reversed(list(re.finditer(r"%{(\w+)}", value))):

It still saves 1 line in the patch, so it's good ;-)

comment:25 by Remy Blank, 8 years ago

Heh, I didn't know that: reversed() can't work with an iterator. Learned one more thing today :)

comment:26 by Remy Blank, 7 years ago

Milestone: 1.01.0-triage

Preparing for 1.0.

comment:27 by Christian Boos, 7 years ago

Move feature requests to next-dev.

comment:28 by Christian Boos, 7 years ago

Milestone: next-stable-1.0.xnext-dev-1.1.x

well, once again… next-dev

comment:29 by Ryan J Ollos, 4 years ago

Milestone: next-dev-1.1.xnext-major-releases

Retargetting tickets to narrow focus for milestone:1.2. Please move the ticket back to milestone:next-dev-1.1.x if you intend to resolve it by milestone:1.2.

comment:30 by Ryan J Ollos, 4 years ago

Owner: Christian Boos removed
Status: reopenednew

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.