[PATCH] trac.ini parameterization support

[sahendrickson@…]

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

[sahendrickson@…]

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

[Christian Boos]

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

[Christian Boos]

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

trac/config.py

@@ -14 +14 @@
 
 from __future__ import with_statement
 
-from ConfigParser import ConfigParser
+from ConfigParser import SafeConfigParser
 from copy import deepcopy
 import os.path
 
@@ -50 +50 @@
     the last modification time of the configuration file, and reparses it
     when the file has changed.
     """
-    def __init__(self, filename):
+    def __init__(self, filename, params={}):
         self.filename = filename
-        self.parser = ConfigParser()
+        self.parser = SafeConfigParser(params)
         self._old_sections = {}
         self.parents = []
         self._lastmtime = 0

trac/env.py

@@ -550 +550 @@
 
     def setup_config(self):
         """Load the configuration file."""
-        self.config = Configuration(os.path.join(self.path, 'conf',
-                                                 'trac.ini'))
+        self.config = Configuration(os.path.join(self.path, 'conf', 'trac.ini'),
+                                    {'envname': os.path.basename(self.path)})
         self.setup_log()
         from trac.loader import load_components
         plugins_dir = self.shared_plugins_dir

[Christian Boos]

Above patch applied in r10565.

[Christian Boos]

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?

[Christian Boos]

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.

[Remy Blank]

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

[Christian Boos]

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

[Remy Blank]

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.

[Christian Boos]

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.

[Remy Blank]

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

[Christian Boos]

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.

[Remy Blank]

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.

[Remy Blank]

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

[Remy Blank]

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

[Remy Blank]

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.

[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. 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.

[Christian Boos]

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 :-)

[osimons]

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.

[osimons]

Patch for interpolation of Option descriptor.

[osimons]

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.

[Remy Blank]

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 :)

[osimons]

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 ;-)

[Remy Blank]

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

[Remy Blank]

Preparing for 1.0.

[Christian Boos]

Move feature requests to next-dev.

[Christian Boos]

well, once again… next-dev

[Ryan J Ollos]

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.