|
Description
David Cook
2020-10-20 00:42:08 UTC
Created attachment 112013 [details] [review] Bug 26742: Add configuration to koha-conf.xml for message broker The connection details for the message broker should be configurable. This patch adds configuration options to koha-conf.xml. If they are not specified, then default connection details will be used. Created attachment 112015 [details] [review] Bug 26742: Add configuration to koha-conf.xml for message broker The connection details for the message broker should be configurable. This patch adds configuration options to koha-conf.xml. If they are not specified, then default connection details will be used. Test plan: 1. Apply the patch 2. koha-plack --restart kohadev 3. Go to http://localhost:8081/cgi-bin/koha/about.pl and check "Message broker" status says "running" 4. Edit /etc/koha/sites/kohadev/koha-conf.xml 5. Add the following: <message_broker> <hostname>localhost</hostname> <port>61613</port> <username>guest</username> <password>guest</password> <vhost></vhost> </message_broker> 6. echo 'flush_all' | nc -q 1 memcached 11211 7. koha-plack --restart kohadev 8. Go to http://localhost:8081/cgi-bin/koha/about.pl and check "Message broker" status says "running" 9. Edit /etc/koha/sites/kohadev/koha-conf.xml 10. Change message broker config to the following: <message_broker> <hostname>badname</hostname> <port>40000</port> <username>guest</username> <password>guest</password> <vhost></vhost> </message_broker> 11. echo 'flush_all' | nc -q 1 memcached 11211 12. koha-plack --restart kohadev 13. Go to http://localhost:8081/cgi-bin/koha/about.pl and check "Message broker" status says "connection failed" Created attachment 113906 [details] [review] Bug 26742: Add configuration to koha-conf.xml for message broker The connection details for the message broker should be configurable. This patch adds configuration options to koha-conf.xml. If they are not specified, then default connection details will be used. Signed-off-by: Michal Denar <black23@gmail.com> Thanks, Michal! Created attachment 114288 [details] [review] Bug 26742: Add configuration to koha-conf.xml for message broker The connection details for the message broker should be configurable. This patch adds configuration options to koha-conf.xml. If they are not specified, then default connection details will be used. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> 1. Wouldn't it make sense to keep "login" instead of "username" in the koha-conf.xml? 2. You are adding a die in the ->connect method, is it really useful? If $stomp->connect fails we already get an exception. (In reply to Jonathan Druart from comment #7) > 1. Wouldn't it make sense to keep "login" instead of "username" in the > koha-conf.xml? > I don't think so, since "username" is more standard than "login". It correlates better with the RabbitMQ documentation for instance (https://www.rabbitmq.com/access-control.html), so I think sysadmins and such would find username more user-friendly. > 2. You are adding a die in the ->connect method, is it really useful? > If $stomp->connect fails we already get an exception. I am very sure that $stomp->connect does not raise an exception when it fails without my patch. Without my patch, when would you get that exception? When it tries to subscribe? if that's the case, I think that's raising an exception too late. There is "Failed to connect: Error connecting to localhost:61613: Cannot assign requested address at /usr/share/perl5/Net/Stomp.pm line 27.; giving up at /usr/share/perl5/Net/Stomp.pm line 27" that is raised by the constructor. So you are right, not by $stomp->connect but already by our Koha::BackgroundJob->connect. It hasn't been advertised by the commit message, I think it need discussion and tests are missing. Can we move this part to its own bug report? (In reply to Jonathan Druart from comment #9) > There is "Failed to connect: Error connecting to localhost:61613: Cannot > assign requested address at /usr/share/perl5/Net/Stomp.pm line 27.; giving > up at /usr/share/perl5/Net/Stomp.pm line 27" that is raised by the > constructor. > So you are right, not by $stomp->connect but already by our > Koha::BackgroundJob->connect. > Hmm, did you get that when RabbitMQ was down? What if RabbitMQ is up but the auth details are wrong? I feel like I didn't get an exception in that latter case. > It hasn't been advertised by the commit message, I think it need discussion > and tests are missing. Can we move this part to its own bug report? I suppose so. I don't really have the stamina at the moment to work on anything, so happy to just have it moved to Failed QA as well. Created attachment 114372 [details] [review] Bug 26742: Remove die from connect The change was not advertised and it needs to be done on a separate bug report (+ test coverage) I think koha-create should handle this. Also, shouldn't we set a default vhost? Yes, changes to koha-create, makefile.pl and rewrite-config.PL are missing. (In reply to Jonathan Druart from comment #13) > Yes, changes to koha-create, makefile.pl and rewrite-config.PL are missing. I think that those can be handled by follow-ups like Bug 25674. Bug 26742 is just moving hard-coded values out of code and into configuration files. (In reply to Tomás Cohen Arazi from comment #12) > I think koha-create should handle this. Also, shouldn't we set a default > vhost? Not specifying a vhost makes it so that the default vhost is used. You could probably achieve the same thing using a value of "/" to explicitly indicate the default vhost, but I haven't tried that specifically. (In reply to David Cook from comment #14) > (In reply to Jonathan Druart from comment #13) > > Yes, changes to koha-create, makefile.pl and rewrite-config.PL are missing. > > I think that those can be handled by follow-ups like Bug 25674. Bug 26742 is > just moving hard-coded values out of code and into configuration files. Go test it :-) (In reply to Tomás Cohen Arazi from comment #16) > Go test it :-) It's on my list! I was tackling more urgent bugs this morning, but I'm getting there ;). (In reply to Tomás Cohen Arazi from comment #16) > Go test it :-) Looks good overall but failed it because I don't think it should be setting a vhost by default. Created attachment 117533 [details] [review] Bug 26742: Add configuration to koha-conf.xml for message broker The connection details for the message broker should be configurable. This patch adds configuration options to koha-conf.xml. If they are not specified, then default connection details will be used. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 117534 [details] [review] Bug 26742: Remove die from connect The change was not advertised and it needs to be done on a separate bug report (+ test coverage) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Tested this as part of testing bug 25674.. so adding my additional signoff line :) Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.04 Missing dependency, not backported to 20.05 |