Bug 808419 : opt-in for user feedback plugin

[ruleant]

This pull request implements feature request in bug #808419.

• It adds a template for a question to be asked by debconf : should the plugin be enabled?
• in the mariadb-server-10.0.postinst script a config file 50-feedback.cnf is created to enable the plugin, by default it is disabled, but if the question is answered with 'true', the plugin is enabled
• debconf takes the current value in the config file as default choice (defaults to 'false' if the value is not found or the config file is missing)
• the mariadb-server-10.0.postinst script handles case when the file is not present, or when it when the required setting was manually changed or removed
• when the package is purged, the configfile is removed
• the language in the template was checked by the debian-l10n-english team.
• TODO : ask translators to translate the template (once the changes are merged with master)

[ottok]

Thank you a lot! I will try to review this soon, but currently I am very busy..

[ruleant]

no problem, take your time. :)

[ottok]

This PR contains commits that edits commits in the PR itself. It would be good practice to squash some of the changes.

Please pull the latest master and then rebase interactively with branch:bug_808419_feedback_plugin$ git rebase -i master and use the edit and squash commands to clean up the PR a bit.

[ottok]

Here is a flawless example of a good PR and git commit messages: #47

You work is good, I can merge as-is if you don't have time to fixup a bit. I just thought that you a perhaps interested in learning to do things perfectly, as you are on path to become a Debian Developer. DD's in general love to perfect the details.

[ruleant]

Thanks for the feedback. I'll rework the PR and squash a few commits.
In order to do it as perfectly as possible, a few questions :

• Is it OK to keep the reference to the bug number in the commit messages?
• Some of the commits can easily be squashed, but for the review of the template I'd like to keep them seperate to keep track of/give credit to the reviewers' input. Alternatively, the reviewers' input could be explained in the commit message. What do you think?

[ottok]

1. I don't know if having the bug number in many commits is good, but at least it is good to have in the last commit "(Closes: #XXXXX)" at the end. This will later be picked up automatically in the the changelog and by bugs.debian.org so they get closed at upload time.

2. You can have the thanks in the commit message. No need to show mistakes+their fixes after review.

[ruleant]

I have cleaned up the commit history by squashing a few commits
Comments are welcome.

[ottok]

All of the changes could fit in one single commit under the title "Add debconf question to enable User Feedback Plugin".

Do you think it would be possible to not echo the config file inside the maint script but instead distribute it to all installations like all the other files under debian/additions/mariadb.conf.d/* are, and only switch it to ON if the user selected so?

[ruleant]

All of the changes could fit in one single commit under the title "Add
debconf question to enable User Feedback Plugin".

Yes, that would be better. I'm getting the hang of 'git rebase
--interactive' so that shouldn't be a problem. :)

Do you think it would be possible to not echo the config file inside the
maint script but instead distribute it to all installations like all the
other files under debian/additions/mariadb.conf.d/* are, and only switch
it to ON if the user selected so?

I gave this some thought when I was investigating on how to create and
modify the config file. Debian Policy mentions that handling config files
with dpkg shouldn't be used when the file is modified by a postinst script :

"Note that a package should not modify a dpkg-handled conffile in its
maintainer scripts. Doing this will lead to dpkg giving the user confusing
and possibly dangerous options for conffile update when the package is
upgraded." [0]

That's why the config file is generated with echo commands.

[0] last paragraph of section E.1 on
https://www.debian.org/doc/debian-policy/ap-pkg-conffiles.html

[ruleant]

All the changes are in one commit now. :)

[ottok]

I read this again now. Looks good. The only thing I wondered about is wheter echoing a config file is a good idea. What if the feedback.cnf config file was part of the packaging and always installed, but with all lines commented out? And then on activation the feedback=on line would have the #-characted stripped off.

Also I wonder if you would be interested to adapt this as a PR against upstream github.com/mariadb/server branch 10.2? I think this would suit perfectly there as 10.2 is in beta stage and feedback is desperately needed there.

[ruleant]

Hi Otto,

Thanks for reviewing.

I read this again now. Looks good. The only thing I wondered about is

wheter echoing a config file is a good idea. What if the feedback.cnf
config file was part of the packaging and always installed, but with all
lines commented out? And then on activation the feedback=on line would have
the #-characted stripped off.

I'd prefer to generate the config file in another way than using a large
echo block in the postinst script, but changing a packaged config file is
not recommend according to Debian Policy (see my comment above :
#46 (comment))

I see some options :

1. echo a smaller config file (only one line with 'feedback=ON')
2. do you know if there is a recommended Debian way to use a templating
   system for generating config files (fe. jinja2, sed)? This would keep the
   echo out of the install script, but the issue when a newer version of a
   generated config file is introduced and keeping user modifications remains

Also I wonder if you would be interested to adapt this as a PR against

upstream github.com/mariadb/server branch 10.2? I think this would suit
perfectly there as 10.2 is in beta stage and feedback is desperately needed
there.

Does mariadb have an installer where the question could be asked?

[ottok]

1. We should really not echo config files. We should have a small feedback.cnf we install is users choose so, otherwise we don't install it. We don't need to edit config files or break the Debian policy in any way.

2. Upstream has a 10.2 branch with a debian/ directory. Check it out. This PR could be adapted to target it and then we can roll-out this in MariaDB 10.2 at upstream.

[ruleant]

1. We should really not echo config files. We should have a small
   feedback.cnf we install is users choose so, otherwise we don't install it.
   We don't need to edit config files or break the Debian policy in any way.

Good point, so I guess the feedback.cnf config file goes into folder
debian/additions/mariadb.conf.d/, so I just need to figure out a way to
conditionally copy it to etc/mysql/mariadb.conf.d (putting it into the
.install file would copy it unconditionally, but I'm not sure if it is part
of the package if it is not in the install file, or if it can be referenced
from the postinst script : a good opportunity to gain some deeper
understanding of the debian package installer

2. Upstream has a 10.2 branch with a debian/ directory. Check it out.
   This PR could be adapted to target it and then we can roll-out this in
   MariaDB 10.2 at upstream.

I assumed as much. Thanks for clarifying.

[ruleant]

Hi Otto,

With some delay, I had time to look into it some more.

Conditionally installing a file based on the result of the debconf question doesn't seem possible. All files mentioned in mariadb-server-10.0.install are installed uncondionally, and the path to unpacked package is not available when the postinst script is run, so copying an uninstalled file from the package is not possible either.

So, there are a few options :

• limit the echoing in postinst to just the required lines, this will make the echoing part look less bulky :

[mysqld]
feedback="ON"

• move part of the postinst script that does the configfile creation to a seperate config script (as suggested in Debian Policy), but that seems a bit overkill for just setting one option.
• create a seperate package (eg. mariadb-server-10.0-feedback) that only installs 1 file (ie. the configfile which enables the feedback plugin), cf. enabling popularity-contest (but then we need to figure out if it's possible to install a package based on the result of a debconf question in another package)

What do you think?

[ottok]

What about installing the file to all users but with the contents commented out? And what the postinstall does, is a simple sed to remove a # character from the line so that after next restart of server the feedback plugin is active?

[ruleant]

I read Debian Policy again on this. What I previously mentioned about not changing an installed config file in a postinst script, does only seem to apply to files listed in a 'conffiles' file (which doesn't exist in the mariadb-server-10.0 package. On a side note, it probably should be added for the other config files that are installed by the package).
So if the feedback config file is not a dpkg-handled configfile, it should be fine to modify it in the postinst script.

The script is currently a bit more complicated (ie. more robust) than removing a #, as it makes sure that the correct setting is added, it also works in cases where the configfile was changed by the user (different setting for the feedback option or removal of the commented feedback option line).

[ruleant]

HI Otto,

I did some more reading. It turns out all files installed in /etc are automatically checked by dpkg as config files. So the only option is to create the configfile using the postinst script, to avoid problems with dpkg complaining or the file being overwritten during and upgrade.

[ruleant]

Another idea :
Install the feedback.conf file in /usr/share/doc/mariadb-server-10.0/ and copy it from there to /etc if the plugin is enable.

[ottok]

Another idea :
Install the feedback.conf file in /usr/share/doc/mariadb-server-10.0/ and copy it from there to /etc if the plugin is enable.

This sounds good!

[ruleant]

This pull request is now updated : a template for 50-feedback.cnf is installed in /usr/share/mariadb-server10.0/examples/ and copied to /etc/mysql/mariadb.conf.d/ by the postinst script.
Depending on the question asked by debconf, the plugin is enabled or not.

There is an alternative version that is a bit simpler : it copies and removes the config file depending on the answer in the debconf question. Downside of this approach : any manual changes to /etc/mysql/mariadb.conf.d/50-feedback.cnf are lost if the plugin is disabled by the debconf question.
It can be found here : https://github.com/ruleant/mariadb-10.0/commit/aaa8c10c8247774b1fd6f8a6a6e947705b683a77

Both versions are rebased onto master

[ottok]

Looks good! Now I wonder if I should apply this on ottok/mariadb-10.1 or on mariadb/server/10.2.. I will not upload new versions of 10.0 to Debian anymore. Maybe the best fit would be to send this directly to upstream 10.2. Do you want to make a PR against https://github.com/MariaDB/server/tree/10.2/debian or do you want me to finalize this (and of course keep you as git --author)?

[ruleant]

Well, applying it to 10.2 upstream will cause it to miss the freeze for stretch. But maybe it's better to have it a longer time in testing before it goes into stable (not that it is a very big change).

I can prepare the PR for upstream 10.2. How easy is to apply the patch to 10.2? I mean, how different is the debian/ dir in 10.2 upstream from the one in the 10.0/10.1 Debian package?
The new messages (used in the debconf template) still have to be translated, I was waiting to contact the translator teams until this patch was merged with master, but if we're applying it only to 10.2 upstream I guess it's better to have the translations included when creating the PR.
I'll add a feature branch to the 10.1 repo as a reference for the translator teams and once all translations are finished, the PR for 10.2 upstream can be created.

What do you think?