Edgewall Software
Modify

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#11437 closed enhancement (fixed)

Configuration sections from plugins should be written to trac.ini

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.4
Component: general Version:
Severity: normal Keywords: configuration trac.ini
Cc: hasienda, Jun Omae Branch:
Release Notes:
  • The default values of configuration options are written to trac.ini when a plugin is enabled through the web administration page or using TracAdmin, provided the options don't already exist in trac.ini or one of its parent configurations.
  • # option-name = <inherited> is not written to trac.ini when the option is specified in a parent configuration.
API Changes:

The EnvironmentStub contains a ConfigurationStub object rather than a Configuration object. The ConfigurationStub stores written data in the file_content attribute rather than writing to disk.

Internal Changes:

Description (last modified by Ryan J Ollos)

When a Trac environment is first created the default data is written to trac.ini. However, when installing a plugin the default data is not written to trac.ini.

When the plugin enabled it would be nice to have the default data added to trac.ini. Currently the user must manually type the section and option name if they wish to change any configuration values. I think that most users would expect that the defaults for all configuration values be found in trac.ini.

A quick implementation that is not ideal, but demonstrates the idea is found in log:rjollos.git:t11437.

One issue with the quick implementation is that all of defaults are written to trac.ini, not just those of the plugin being installed. Maybe this is okay, but if a user likes to remove a lot default options to simplify the file, they may be surprised to have their changes undone (#7378).

#8290 had me thinking of a different solution, that plugins could add the information through existing IEnvironmentParticipant method, or new methods or helper functions that we add. The downside of course is that every plugin then needs to be modified to implement the behavior.

Side note: it might be useful to have a trac-admin $ENV config clean command to remove options from trac.ini that are no longer defined in an enabled Component. For example, with th:AccountManagerPlugin, many options were renamed going from 0.3 → 0.5, and the users must manually clean up these sections. This comes up rather frequently when users post their configuration to tickets and it contains a mess of old and new configuration options. Of course, it causes no harm to leave them, it is just more confusing and error prone to manage the trac.ini file. We could, by default, create a backup of the trac.ini file whenever clean is run.

Attachments (0)

Change History (25)

in reply to:  description comment:1 by Jun Omae, 10 years ago

Cc: Jun Omae added

A quick implementation that is not ideal, but demonstrates the idea is found in log:rjollos.git:t11437.

Another PoC: jomae.git@t11437.1.

comment:2 by Ryan J Ollos, 10 years ago

Description: modified (diff)
Milestone: undecided1.0.2

The changes in comment:1 look good to me. I'm testing them along with the changes in #11520 and I'll make some comments in that ticket.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:3 by anonymous, 10 years ago

FWIW, Please keep in mind the use of the "global configuration". I use this extensively, and I keep most settings in the global configuration file.

For instance, if updating some setting in a Trac instance introduced a lot of other kinds of settings into the local configuration file, it could be then necessary to clean out all the defaults overriding the global configuration.

(I mainly mention this due to the comment about writing all defaults to trac.ini, not just those of the plugin being installed.)

comment:4 by Ryan J Ollos, 10 years ago

The latest changes after #11520 can be found in log:rjollos.git:t11437.2. However, I plan to do more work before committing.

comment:5 by S.N.Maijers@…, 10 years ago

I am not convinced of the usefulness of this. What happens if the configuration format for a plugin changes? Why would an administrator expect default options to be explicitly in the Trac configuration? The current system, where the administrator may set/override (default) configuration options works fine and is more robust in my opinion.

in reply to:  5 comment:6 by Ryan J Ollos, 10 years ago

Replying to S.N.Maijers@…:

What happens if the configuration format for a plugin changes?

The same thing that would happen if the option has been added by editing the config file. The extra entries would be harmless and are the use case for the config clean command that I describe in comment:description.

Why would an administrator expect default options to be explicitly in the Trac configuration?

Less typing and need to look up what options that are available in the documentation. Every default option for Trac is written to the config file, so adding plugin defaults would be consistent.

The current system, where the administrator may set/override (default) configuration options works fine and is more robust in my opinion.

You've not made any argument for why the change would not be robust.

Version 0, edited 10 years ago by Ryan J Ollos (next)

comment:7 by Ryan J Ollos, 10 years ago

Status: newassigned

in reply to:  3 comment:8 by jholg@…, 10 years ago

Replying to anonymous:

FWIW, Please keep in mind the use of the "global configuration". I use this extensively, and I keep most settings in the global configuration file.

+1 Please take into account multi-project trac installations where

  • there's one "master" project and all the "slave" projects inherit its global configuration, overriding only those config options that differ
  • plugins are installed globally i.e. the Python installation or virtualenv used for running Trac, not in a project's plugins dir

Changes introduced with this ticket should not lead to writing defaults to "slave" trac.ini files

comment:9 by Ryan J Ollos, 10 years ago

Milestone: 1.0.21.0.3

comment:10 by Ryan J Ollos, 9 years ago

Milestone: 1.0.31.1.3

comment:11 by Ryan J Ollos, 9 years ago

Milestone: 1.1.31.1.4

comment:12 by Ryan J Ollos, 9 years ago

Proposed changes in log:rjollos.git:t11437.4. The following behaviors have been implemented:

  • When a plugin is enabled through TracAdmin or the Web administration page, the component options are written to trac.ini if they don't already exists in trac.ini or a parent configuration.
  • The # option-name = <inherited> markers are no longer written to trac.ini, but still if the value is defined in a parent configuration, the default value is not written to trac.ini when enabling a component or creating an environment.

comment:13 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

comment:14 by Jun Omae, 9 years ago

Reviewing.

  • If possible, I think we should add unit tests for Configuration.set_defaults(component=...) with and without [inherit] file = ....
  • If the component argument is tracopt.ticket.commit_updater.Commit, the method would wrongly set defaults of options in CommitTicketUpdater. My PoC is wrong.
    • trac/config.py

      diff --git a/trac/config.py b/trac/config.py
      index 6bcf5d7..f56a1dc 100644
      a b class Configuration(object):  
      314314
      315315        if component:
      316316            if component.endswith('.*'):
      317                 component = component[:-1]
      318             component = component.lower()
       317                component = component[:-2]
       318            component = component.lower().split('.')
      319319            from trac.core import ComponentMeta
      320320            for cls in ComponentMeta._components:
      321                 clsname = (cls.__module__ + '.' + cls.__name__).lower()
      322                 if not clsname.startswith(component) and clsname != component:
       321                clsname = (cls.__module__ + '.' + cls.__name__).lower() \
       322                                                               .split('.')
       323                if clsname[:len(component)] != component and \
       324                        clsname != component:
      323325                    continue
      324326                for option in cls.__dict__.itervalues():
      325327                    if not isinstance(option, Option):

comment:15 by Ryan J Ollos, 9 years ago

Additional refactoring and tests added in log:rjollos.git:t11437.4. There are some test failures when running $ make test=trac/tests/__init__.py, which are due to the other Components defined in trac.tests.config. I'm thinking about how to modify or restructure the tests to avoid the failures, but I welcome ideas on that.

comment:16 by Jun Omae, 9 years ago

The Configuration.set_defaults method uses registered classes in ComponentMeta._components. I think we should clean up the class instances to remove test classes and interfaces in the tearDown of unit tests. ConfigSection.registry and ComponentMeta._registry have the same issue. See [0ba577eb4/jomae.git].

Minor thing, component.rstrip('.*') is used, Environment class strips only the trailing .* of component name in tags/trac-1.1.3/trac/env.py@:385#L382. I think we should use the same logic.

Last edited 9 years ago by Jun Omae (previous) (diff)

in reply to:  16 ; comment:17 by Ryan J Ollos, 9 years ago

Replying to jomae:

The Configuration.set_defaults method uses registered classes in ComponentMeta._components. I think we should clean up the class instances to remove test classes and interfaces in the tearDown of unit tests. ConfigSection.registry and ComponentMeta._registry have the same issue. See [0ba577eb4/jomae.git].

Thanks, I was trying to accomplish that but having no success.

If I run the tests using either of these two commands, ConfigurationSetDefaultsTestCase is skipped:

$ PYTHONPATH=. make test=trac/tests/config.py
$ PYTHONPATH=. python test/tests/config.py
SKIP: trac.tests.config.ConfigurationSetDefaultsTestCase (__name__ is not trac.tests.config)

Those two are the same of course because Makefile runs python test/tests/config.py. The tests get run for this command though:

$ python setup.py test -s trac.tests.config

Is that an expected limitation? If so, maybe we should modify how our Makefile runs the tests.

Minor thing, component.rstrip('.*') is used, Environment class strips only the trailing .* of component name in tags/trac-1.1.3/trac/env.py@:385#L382. I think we should use the same logic.

Does that mean that trac.tests.config. should not cause the components in the trac.tests.config to be enabled? I suspect yes, since Trac enables the components in module1 with the following rules:

module1 = enabled
module1.* = enabled

but not with:

module1. = enabled

I guess that was probably your point though in saying that we should replicate the rule that exists in trac.env.

in reply to:  17 comment:18 by Jun Omae, 9 years ago

Is that an expected limitation? If so, maybe we should modify how our Makefile runs the tests.

Yes. Running python trac/tests/config.py, CompA and CompB classes are defined under __main__ package. Then, all tests of ConfigurationSetDefaultsTestCase would fail. Similar discussion in comment:11:ticket:11312.

Minor thing, component.rstrip('.*') is used, Environment class strips only the trailing .* of component name in tags/trac-1.1.3/trac/env.py@:385#L382. I think we should use the same logic.

Does that mean that trac.tests.config. should not cause the components in the trac.tests.config to be enabled? I suspect yes, since Trac enables the components in module1 with the following rules:

Right. modname1. = enabled wouldn't enable components in modname1 package.

in reply to:  17 ; comment:19 by Jun Omae, 9 years ago

Those two are the same of course because Makefile runs python test/tests/config.py. The tests get run for this command though:

$ python setup.py test -s trac.tests.config

After the following patch, the Makefile would convert the file name to module name. We could run unit tests with make test=trac/tests/config.py and make trac/tests/config.py.

  • Makefile

    diff --git a/Makefile b/Makefile
    index dcee889..7eef4d7 100644
    a b export HELP_CFG  
    124124
    125125.PHONY: all help status clean clean-bytecode clean-mo
    126126
     127%.py : status
     128        python -m $(subst /,.,$(@:.py=)) $(testopts)
     129
    127130ifdef test
    128131all: status
    129         python $(test) $(testopts)
     132        python -m $(subst /,.,$(test:.py=)) $(testopts)
    130133else
    131134all: help
    132135endif

in reply to:  19 ; comment:20 by Ryan J Ollos, 9 years ago

Replying to jomae:

After the following patch, the Makefile would convert the file name to module name. We could run unit tests with make test=trac/tests/config.py and make trac/tests/config.py.

The ConfigurationSetDefaultsTestCase tests are still skipped since __name__ == __main__ in the module. Is there another complementary change that I should make?

With the following variation of your patch the ConfigurationSetDefaultsTestCase are executed using make test=trac/tests/config.py and make trac/tests/config.py.

  • Makefile

    diff --git a/Makefile b/Makefile
    index 4c5d3a0..83f82c6 100644
    a b export HELP_CFG  
    125125.PHONY: all help status clean clean-bytecode clean-mo
    126126
    127127%.py : status
    128        python setup.py -q test -s $(subst /,.,$(@:.py=)) $(testopts)
     128       python setup.py -q test -s $(subst /,.,$(@:.py=)).suite $(testopts)
    129129
    130130ifdef test
    131131all: status
    132        python setup.py -q test -s $(subst /,.,$(test:.py=)) $(testopts)
     132       python setup.py -q test -s $(subst /,.,$(test:.py=)).suite $(testopts)
    133133else
    134134all: help
    135135endif

I haven't done enough testing yet to be confident of the change.

in reply to:  20 comment:21 by Jun Omae, 9 years ago

Replying to rjollos:

The ConfigurationSetDefaultsTestCase tests are still skipped since __name__ == __main__ in the module. Is there another complementary change that I should make?

Sorry. I was confused with something else.

With the following variation of your patch the ConfigurationSetDefaultsTestCase are executed using make test=trac/tests/config.py and make trac/tests/config.py.

[…]

I haven't done enough testing yet to be confident of the change.

That patch seems to be good to me.

comment:22 by Ryan J Ollos, 9 years ago

Thanks for the review. I committed against #11316 since the change fixes the issue in that ticket as well. I'm thinking we should suggest in TracDev/UnitTests to use the Makefile for test execution rather than suggest so many different ways to execute the tests, some of which can fail for some modules.

I'm also wondering if there might be a better way yet. If we use python -m unittest to run the tests, we could directly specify a single class or method to execute.

$ PYTHONPATH=. python -m unittest trac.tests.config
.............................................
----------------------------------------------------------------------
Ran 45 tests in 1.621s

OK
$ PYTHONPATH=. python -m unittest trac.tests.config.ConfigurationTestCase
......................................
----------------------------------------------------------------------
Ran 38 tests in 2.068s

OK
$ PYTHONPATH=. python -m unittest trac.tests.config.ConfigurationTestCase.test_repr
.
----------------------------------------------------------------------
Ran 1 test in 0.017s

OK
  • Makefile

    diff --git a/Makefile b/Makefile
    index 83f82c6..600325a 100644
    a b export HELP_CFG  
    125125.PHONY: all help status clean clean-bytecode clean-mo
    126126
    127127%.py : status
    128        python setup.py -q test -s $(subst /,.,$(@:.py=)).suite $(testopts)
     128       python -m unittest $(subst /,.,$(@:.py=)) $(testopts)
    129129
    130130ifdef test
    131131all: status
    132        python setup.py -q test -s $(subst /,.,$(test:.py=)).suite $(testopts)
     132       python -m unittest $(subst /,.,$(test:.py=)) $(testopts)
    133133else
    134134all: help
    135135endif

However, there are test failures when running trac/wiki/tests/formatter.py, and no tests run when specifying trac/wiki/tests/__init__.py. Perhaps that is worth investigating more down the road.

comment:23 by Ryan J Ollos, 9 years ago

Proposed changes in log:rjollos.git:t11437.7.

comment:24 by Ryan J Ollos, 9 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [13859:13860].

in reply to:  description comment:25 by Ryan J Ollos, 9 years ago

Replying to rjollos:

Side note: it might be useful to have a trac-admin $ENV config clean command to remove options from trac.ini that are no longer defined in an enabled Component.

Related idea in comment:15:ticket:6551.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


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