Bug 20692

Summary: koha-plack doesn't check for Include *-plack.conf line in /etc/apache2/sites-available/$INSTANCE.conf
Product: Koha Reporter: Piotr Wejman <ropuch>
Component: Command-line UtilitiesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: minor    
Priority: P5 - low CC: jonathan.druart, lucas, martin.renvoize, mtompset, nick, robin
Version: Main   
Hardware: All   
OS: Linux   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05
Attachments: Bug 20692: Return and display and error if koha-plack failed
Bug 20692: Return and display and error if koha-plack failed
Bug 20692: Return and display and error if koha-plack failed
Bug 20692: Return and display and error if koha-plack failed
Bug 20692: Return and display and error if koha-plack failed
Bug 20692: Extreme toggle checking for plack
Bug 20692: Extreme toggle checking for plack
Bug 20692: Extreme toggle checking for plack

Description Piotr Wejman 2018-05-01 20:26:39 UTC
When enabling Plack, koha-plack uncomment plack related Include in instance's apache conf. Little problem occurs when old pre-plack configs are used, without required Include lines. In this case koha-plack still report success, with no change done.

So in the end we have running Plack, but it's doing nothing. plack.log is empty, but there is an entry in plack-error.log that can trick people into thinking they have Plack properly configured and running:

2018/05/01-21:06:27 Starman::Server (type Net::Server::PreFork) starting! pid(10257)
Binding to UNIX socket file "/var/run/koha/galway/plack.sock"
Setting gid to "1001 1001"
Setting uid to "1001"

koha-plack:enable_plack() should check if Plack lines exists before trying to uncomment them and either fail with warn message, or add them.
Comment 1 Jonathan Druart 2018-05-03 19:43:05 UTC
What says the about page (tab "system information")?
Comment 2 Jonathan Druart 2018-05-03 20:13:03 UTC
Created attachment 75038 [details] [review]
Bug 20692: Return and display and error if koha-plack failed

If koha-plack did not comment/uncomment the line it should fails and
display an error instead of success.

Useful for old apache config files
Comment 3 Jonathan Druart 2018-05-03 20:22:16 UTC
Created attachment 75040 [details] [review]
Bug 20692: Return and display and error if koha-plack failed

If koha-plack did not comment/uncomment the line it should fails and
display an error instead of success.

Useful for old apache config files
Comment 4 Jonathan Druart 2018-05-23 18:06:46 UTC
Created attachment 75510 [details] [review]
Bug 20692: Return and display and error if koha-plack failed

If koha-plack did not comment/uncomment the line it should fails and
display an error instead of success.

Useful for old apache config files
Comment 5 Jonathan Druart 2018-05-23 18:08:45 UTC
Created attachment 75511 [details] [review]
Bug 20692: Return and display and error if koha-plack failed

If koha-plack did not comment/uncomment the line it should fails and
display an error instead of success.

Useful for old apache config files
Comment 6 Mark Tompsett 2018-05-23 18:48:27 UTC
The 'q' portion of the sed logic is faulty.
Comment 7 Jonathan Druart 2018-05-28 14:57:03 UTC
Created attachment 75594 [details] [review]
Bug 20692: Return and display and error if koha-plack failed

If koha-plack did not comment/uncomment the line it should fails and
display an error instead of success.

Useful for old apache config files
Comment 8 Jonathan Druart 2018-05-28 14:57:52 UTC
Well, this is really not error proof but should improve a bit the behavior (not the code!)
Comment 9 Mark Tompsett 2018-05-28 15:05:30 UTC
(In reply to Jonathan Druart from comment #8)
> Well, this is really not error proof but should improve a bit the behavior
> (not the code!)

Hmmm... it gives me an idea... I'll see if I can create a better counter patch, if not, I'll sign this off.
Comment 10 Mark Tompsett 2018-05-28 17:51:24 UTC
Created attachment 75597 [details] [review]
Bug 20692: Extreme toggle checking for plack

TEST PLAN
---------
sudo koha-plack --disable
sudo koha-plack --disable
sudo koha-plack --enable

sudo koha-plack --disable
echo $?
sudo koha-plack --disable
echo $?
sudo koha-list --noplack
echo $?
sudo koha-plack --enable
echo $?
sudo koha-plack --enable
echo $?
sudo koha-list --plack
echo $?

git bz apply 20692
sudo perl ~/misc4dev/cp_debian_files.pl
then repeat the test.

It should also be noted, because the enable
and disable are now split, if for some reason
they become out of sync, an enable or disable
should correct them.

Because these are bash scripts, the test tool
(/home/vagrant/qa-test-tools/koha-qa.pl)
does not apply.
Comment 11 Liz Rea 2019-04-16 15:42:31 UTC
Created attachment 88085 [details] [review]
Bug 20692: Extreme toggle checking for plack

TEST PLAN
---------
sudo koha-plack --disable
sudo koha-plack --disable
sudo koha-plack --enable

sudo koha-plack --disable
echo $?
sudo koha-plack --disable
echo $?
sudo koha-list --noplack
echo $?
sudo koha-plack --enable
echo $?
sudo koha-plack --enable
echo $?
sudo koha-list --plack
echo $?

git bz apply 20692
sudo perl ~/misc4dev/cp_debian_files.pl
then repeat the test.

It should also be noted, because the enable
and disable are now split, if for some reason
they become out of sync, an enable or disable
should correct them.

Because these are bash scripts, the test tool
(/home/vagrant/qa-test-tools/koha-qa.pl)
does not apply.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 12 Martin Renvoize 2019-04-25 07:30:49 UTC
Created attachment 88698 [details] [review]
Bug 20692: Extreme toggle checking for plack

TEST PLAN
---------
sudo koha-plack --disable
sudo koha-plack --disable
sudo koha-plack --enable

sudo koha-plack --disable
echo $?
sudo koha-plack --disable
echo $?
sudo koha-list --noplack
echo $?
sudo koha-plack --enable
echo $?
sudo koha-plack --enable
echo $?
sudo koha-list --plack
echo $?

git bz apply 20692
sudo perl ~/misc4dev/cp_debian_files.pl
then repeat the test.

It should also be noted, because the enable
and disable are now split, if for some reason
they become out of sync, an enable or disable
should correct them.

Because these are bash scripts, the test tool
(/home/vagrant/qa-test-tools/koha-qa.pl)
does not apply.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-04-25 07:31:24 UTC
Nice bit of code curling there guys, a solid improvement, thanks.

Passing QA
Comment 14 Nick Clemens 2019-04-25 11:52:12 UTC
Awesome work all!

Pushed to master for 19.05
Comment 15 Martin Renvoize 2019-04-26 15:28:08 UTC
Pushed to 18.11.x for 18.11.05
Comment 16 Lucas Gass 2019-05-24 15:46:34 UTC
backported to 18.05.x for 18.05.13