Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate conf_2_db feature #1424

Closed
3 tasks done
ikedas opened this issue Jun 15, 2022 · 4 comments · Fixed by #1492
Closed
3 tasks done

Deprecate conf_2_db feature #1424

ikedas opened this issue Jun 15, 2022 · 4 comments · Fixed by #1492
Labels
design enhancement ready A PR is waiting to be merged. Close to be solved
Milestone

Comments

@ikedas
Copy link
Member

ikedas commented Jun 15, 2022

Expected Behavior

The feature to save configuration into database table should be deprecated.

Current Behavior

There are two use cases:

  • Content of sympa.conf and/or robot.conf may be imported into conf_table in database with sympa.pl --conf_2_db, and they may override default values of configuration.
    However, configuration in database affects only sympa_msg.pl and sympa_automatic.pl daemons and sympa.pl command line utility. The other daemons and most of command line tools seem not affected.

  • Color setting (color_* parameters) on web UI are saved into conf_table, and they override configuration in each robot.conf.
    These settings are shared by possible multiple FastCGI servers through the database.

Possible Solution

  • Deprecate conf_2_db feature to import configuration into database table.
  • The setting imported into database should be exported to the configuration files during upgrade process.
  • Color setting will be kept in the database.

[EDIT] It turned out that this feature has not dealt with the parameters other than CSS colors, so exporting parameters to the configuration files is unnecessary.

Context

This feature was introduced in Sympa 6.1. cf. the initial commit.

Probably it has been rarely used, except for color settings.

Deprecating this, configuration will be independent from database. It will make the code simpler.

@racke
Copy link
Contributor

racke commented Jun 16, 2022

Sounds good to me 👍

@dverdin
Copy link
Contributor

dverdin commented Jun 23, 2022

I precisely just used it for color settings. :-)

It is useful when, like I do, you regularly import new robots, when people are migrating from self hosting to a shared server.
Of course, most of the time, it involves an upgrade because we rarely run the same version of Sympa, so what you propose would probably work.

I remember why the conf_2_db feature had been asked: It was for people running a very large number of virtual hosts (I'm speaking thousands of robots). When reloading Sympa processes, reading the config files seemed to be a problem. I don't know why it was not applied to all Sympa daemons. Just a history note so you get some context.

Anyway, while running this command, I noticed a few bugs (introduced a long time ago). Is it still useful that I propose the fixes?
they are here: sympa-6.2...dverdin:fix-conf_2_db

@ikedas
Copy link
Member Author

ikedas commented Jun 23, 2022

OK. It turns out that this is one of the features that was only planned but not working properly.

In these years, I have been working on fixing many of those things to make them really usable or removing them to cleanup code. I am happy to be able to safely remove this (unfinished) feature since it has never been used (except for color settings) in the first place.

@racke
Copy link
Contributor

racke commented Jun 24, 2022

Yes that makes sense to me.

@ikedas ikedas added the ready A PR is waiting to be merged. Close to be solved label Oct 7, 2022
@racke racke added this to the 6.2.72 milestone Nov 5, 2022
ikedas added a commit that referenced this issue Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design enhancement ready A PR is waiting to be merged. Close to be solved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants