bugfixes, cleanups and improvements #10
Conversation
…mments in the conf file
…eded, but not in sympa.conf
| @@ -335,6 +335,7 @@ cp -p %{SOURCE113} __doc/README.RPM.md | |||
| %else | |||
| cp -p %{SOURCE112} __doc/README.RPM.md | |||
| %endif | |||
| mv %{buildroot}%{_sysconfdir}/sympa/README __doc/ | |||
There was a problem hiding this comment.
CONFDIR/README (etc_README in source) describes usage of CONFDIR and DEFAULTDIR. So it would be on either of these directories.
There was a problem hiding this comment.
I understand the rationale for putting this in either (or both) of configdir and defaultdir, but documentation definitely belongs to docdir. Maybe I could move the file to docdir, but also symlink it from both configdir and defaultdir ?
| @@ -291,6 +291,9 @@ pushd po/web_help; rm -f stamp-po; make; popd | |||
| # Fix perm to prevent warning by rpmlint. | |||
| chmod a-x %{buildroot}%{_datadir}/%{name}/bin/create_db.Sybase | |||
|
|
|||
| # Remove useless ca-bundle.crt | |||
There was a problem hiding this comment.
This file is referred by the code (see Conf.pm). It would be better to propose upstream removal.
There was a problem hiding this comment.
Yes, I noticed the issue and worked around it by adding a symlink.
I've filled an issue about this upstream, as you suggested.
sympa-community/sympa#116
SPECS/sympa-6.2.spec.in
Outdated
| @@ -342,6 +342,8 @@ cp -p %{SOURCE113} __doc/README.RPM.md | |||
| cp -p %{SOURCE112} __doc/README.RPM.md | |||
| %endif | |||
| mv %{buildroot}%{_sysconfdir}/sympa/README __doc/ | |||
| # Remove smtpc.1.md (this should be made a manpage instead) | |||
| rm __doc/smtpc.1.md | |||
There was a problem hiding this comment.
There is no manpage: smtpc.1.md is only documentation.
There was a problem hiding this comment.
Ok, I will drop this commit.
| if [ '!' -e %{_sysconfdir}/sympa/scenari/$scenario ]; then | ||
| touch %{_sysconfdir}/sympa/scenari/$scenario:ignore | ||
| fi | ||
| done |
There was a problem hiding this comment.
intranet settings are incomplete examples and it would be better to disable them by default.
There was a problem hiding this comment.
I guess the removal should be done upstream, I've filed an issue about this :
sympa-community/sympa#119
Meanwhile, I'll also remove the intranet related files (default/create_list_templates/intranet_list and default/scenari/intranet) in %install, which should have the same effect than the removed code.
(it would be better to remove the files in %prep, but then configure complains, as the Makefile needs to be tweaked too)
SPECS/sympa-6.2.spec.in
Outdated
| @@ -547,17 +562,26 @@ fi | |||
| %license COPYING | |||
| %dir %attr(-,sympa,sympa) %{_sysconfdir}/sympa/ | |||
| %config(noreplace) %attr(0640,sympa,sympa) %{_sysconfdir}/sympa/sympa.conf | |||
| %config(missingok) %{_sysconfdir}/sympa/create_list_templates | |||
There was a problem hiding this comment.
Files under _sysconfdir (except sympa.conf) are optional: Files under DIFAULTDIR are used by default.
And any or most of these files can be removed or moved by upstream in the future.
So I suppose missingok is better than noreplace.
There was a problem hiding this comment.
Having all the config files available make it easier to understand how to tune sympa behaviour, so I'd like to keep them. But as you outlined, these config files are actually not mandatory, so I'll go with %config(noreplace,missingok) which should allow for both. If file are removed or moved upstream, it would need to be acommodated for in the specfile anyway, so this shouldn't be an issue.
SOURCES/sympa-httpd-fcgid-el7.conf
Outdated
| MaxProcessCount 5 | ||
| MaxRequestLen 131072 | ||
| FcgidMaxProcesses 5 | ||
| FcgidMaxRequestLen 131072 |
There was a problem hiding this comment.
The rationale for keeping FcgidIOTimeout and FcgidMaxRequestLen but discard FcgidMaxProcesses is the formers are valid inside a VirtualHost while the later is not.
I'll add another patch to plain remove them and add a comment.
| @@ -13,7 +13,107 @@ package. A bunch of work is needed to start your Sympa service. | |||
| Sympa and updates site configuration files. In this process you must | |||
| choose e-mail(s) of listmaster, database settings and so on. | |||
|
|
|||
| 2 Setup database | |||
There was a problem hiding this comment.
Configuration can refer database. So I suppose database setup would be better to be done at first.
There was a problem hiding this comment.
My point is if one follows the steps in README.RPM closely and setup the database before setting up the DB, he will not get the mail to sympa@domain.tld sent by 'sympa.pl --health_check', thus the MTA needs to be setup before the DB.
|
I've created a new branch PR-20171108 hopefully addressing your comments and some more fixup, but I don't know how to update the pull request to target the new branch. Sorry, not used to github workflow, I'm probably not doing that in the right way at all... |
1 similar comment
|
I've created a new branch PR-20171108 hopefully addressing your comments and some more fixup, but I don't know how to update the pull request to target the new branch. Sorry, not used to github workflow, I'm probably not doing that in the right way at all... |
|
This PR is replaced by #13 |
Hi Soji,
Here's a pile of patches for you to review. I expect most of them to be fine as is, but there is a non null probability I'm missing some background and I'm indeed open to discussion :-)
I have one more patch pending in my WIP branch, which changes the location and handling of sympa aliases. As such, it is a bit more disruptive and needs to be discussed.
Regards,
Xavier