Bug 25172 - Koha::Logger init is failing silently
Summary: Koha::Logger init is failing silently
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 14167
Blocks: 15253 25284 25527
  Show dependency treegraph
 
Reported: 2020-04-16 14:08 UTC by Jonathan Druart
Modified: 2020-05-18 12:21 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 25172: Make Koha::Logger explode if init went wrong (1.48 KB, patch)
2020-04-16 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Remove _recheck_logfile (2.03 KB, patch)
2020-04-16 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Adjust tests (2.84 KB, patch)
2020-04-16 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Identify and display possible problems on the about page (4.99 KB, patch)
2020-04-16 15:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Make Koha::Logger explode if init went wrong (2.94 KB, patch)
2020-04-20 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Remove _recheck_logfile (2.03 KB, patch)
2020-04-20 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Adjust tests (2.84 KB, patch)
2020-04-20 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Identify and display possible problems on the about page (4.99 KB, patch)
2020-04-20 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25172: Make Koha::Logger explode if init went wrong (3.01 KB, patch)
2020-04-20 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25172: Remove _recheck_logfile (2.09 KB, patch)
2020-04-20 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25172: Adjust tests (2.90 KB, patch)
2020-04-20 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25172: Identify and display possible problems on the about page (5.05 KB, patch)
2020-04-20 14:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25172: Add missing filter (1.12 KB, patch)
2020-04-20 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25172: Make Koha::Logger explode if init went wrong (3.08 KB, patch)
2020-04-21 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25172: Remove _recheck_logfile (2.16 KB, patch)
2020-04-21 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25172: Adjust tests (2.97 KB, patch)
2020-04-21 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25172: Identify and display possible problems on the about page (5.12 KB, patch)
2020-04-21 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25172: Add missing filter (1.17 KB, patch)
2020-04-21 16:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25172: Fix Auth_with_ldap.t (985 bytes, patch)
2020-04-22 14:41 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-04-16 14:08:45 UTC
This is terribly wrong as we are not aware the logger did not initiated correctly and we are going to lose logs.

We should indeed make sure the error will be blocking and propagated to the interface.
Comment 1 Jonathan Druart 2020-04-16 14:53:40 UTC
Created attachment 103088 [details] [review]
Bug 25172: Make Koha::Logger explode if init went wrong

We do not want to fail silently for the logger.
Comment 2 Jonathan Druart 2020-04-16 14:53:43 UTC
Created attachment 103089 [details] [review]
Bug 25172: Remove _recheck_logfile

I don't understand how that could be useful. We do not want to test if
the logfile is writable every time we log something!
Comment 3 Jonathan Druart 2020-04-16 14:53:46 UTC
Created attachment 103090 [details] [review]
Bug 25172: Adjust tests
Comment 4 Jonathan Druart 2020-04-16 15:17:19 UTC
Created attachment 103091 [details] [review]
Bug 25172: Identify and display possible problems on the about page
Comment 5 Kyle M Hall 2020-04-20 13:22:00 UTC
Applying: Bug 25172: Make Koha::Logger explode if init went wrong
error: sha1 information is lacking or useless (Koha/Logger.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 25172: Make Koha::Logger explode if init went wrong

I think this patch set needs a rebase.
Comment 6 Jonathan Druart 2020-04-20 14:01:06 UTC
Created attachment 103300 [details] [review]
Bug 25172: Make Koha::Logger explode if init went wrong

We do not want to fail silently for the logger.
Comment 7 Jonathan Druart 2020-04-20 14:01:09 UTC
Created attachment 103301 [details] [review]
Bug 25172: Remove _recheck_logfile

I don't understand how that could be useful. We do not want to test if
the logfile is writable every time we log something!
Comment 8 Jonathan Druart 2020-04-20 14:01:12 UTC
Created attachment 103302 [details] [review]
Bug 25172: Adjust tests
Comment 9 Jonathan Druart 2020-04-20 14:01:15 UTC
Created attachment 103303 [details] [review]
Bug 25172: Identify and display possible problems on the about page
Comment 10 Martin Renvoize 2020-04-20 14:42:47 UTC
Patches apply now, taking a look.
Comment 11 Martin Renvoize 2020-04-20 14:51:13 UTC
Created attachment 103304 [details] [review]
Bug 25172: Make Koha::Logger explode if init went wrong

We do not want to fail silently for the logger.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-04-20 14:51:17 UTC
Created attachment 103305 [details] [review]
Bug 25172: Remove _recheck_logfile

I don't understand how that could be useful. We do not want to test if
the logfile is writable every time we log something!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-04-20 14:51:20 UTC
Created attachment 103306 [details] [review]
Bug 25172: Adjust tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-04-20 14:51:23 UTC
Created attachment 103307 [details] [review]
Bug 25172: Identify and display possible problems on the about page

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-04-20 15:29:06 UTC
This all appears to work as expected for me.. and offers a genuine improvement.. I didn't like that we just carried on regardless either.. I'm sure Kyle has also done similar changes in a SIP bug at some point?
Comment 16 Kyle M Hall 2020-04-20 16:43:05 UTC
(In reply to Martin Renvoize from comment #15)
> This all appears to work as expected for me.. and offers a genuine
> improvement.. I didn't like that we just carried on regardless either.. I'm
> sure Kyle has also done similar changes in a SIP bug at some point?

The SIP logging bug uses Koha::Logger so its' behavior should be fixed by this as well.
Comment 17 Kyle M Hall 2020-04-20 17:12:47 UTC
I love everything about this patch set, but I'm getting one unit test failure:

root@6b3a1a3db7b8:koha(bug25172-qa)$ prove t/Logger.t 
t/Logger.t .. 
    #   Failed test 'Logger did not init correctly without permission'
    #   at t/Logger.t line 52.
    # Looks like you failed 1 test of 6.
t/Logger.t .. 1/1 
#   Failed test 'Test01 -- Simple tests for Koha::Logger'
#   at t/Logger.t line 63.
# Looks like you failed 1 test of 1.
t/Logger.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

Test Summary Report
-------------------
t/Logger.t (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=1,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.36 cusr  0.06 csys =  0.44 CPU)
Result: FAIL


Jonathan, does this test fail for you as well?
Comment 18 Kyle M Hall 2020-04-20 17:16:25 UTC
Created attachment 103310 [details] [review]
Bug 25172: Add missing filter
Comment 19 Jonathan Druart 2020-04-21 08:32:19 UTC
(In reply to Kyle M Hall from comment #17)
> I love everything about this patch set, but I'm getting one unit test
> failure:
> 
> root@6b3a1a3db7b8:koha(bug25172-qa)$ prove t/Logger.t 
> t/Logger.t .. 
>     #   Failed test 'Logger did not init correctly without permission'
>     #   at t/Logger.t line 52.
>     # Looks like you failed 1 test of 6.
> t/Logger.t .. 1/1 
> #   Failed test 'Test01 -- Simple tests for Koha::Logger'
> #   at t/Logger.t line 63.
> # Looks like you failed 1 test of 1.
> t/Logger.t .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/1 subtests 
> 
> Test Summary Report
> -------------------
> t/Logger.t (Wstat: 256 Tests: 1 Failed: 1)
>   Failed test:  1
>   Non-zero exit status: 1
> Files=1, Tests=1,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.36 cusr  0.06
> csys =  0.44 CPU)
> Result: FAIL
> 
> 
> Jonathan, does this test fail for you as well?

Thanks for testing Kyle.
No, it passes for me. But I have a weird behavior with this permission test, I had to move it up before the other test (when the log file is 700).
Comment 20 Martin Renvoize 2020-04-21 09:38:39 UTC
Hmm, I can't get the test to fail either.
Comment 21 Kyle M Hall 2020-04-21 16:26:48 UTC
(In reply to Martin Renvoize from comment #20)
> Hmm, I can't get the test to fail either.

Thanks for the confirmations. It's probably something odd with my setup.
Comment 22 Kyle M Hall 2020-04-21 16:28:22 UTC
Created attachment 103391 [details] [review]
Bug 25172: Make Koha::Logger explode if init went wrong

We do not want to fail silently for the logger.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2020-04-21 16:28:39 UTC
Created attachment 103392 [details] [review]
Bug 25172: Remove _recheck_logfile

I don't understand how that could be useful. We do not want to test if
the logfile is writable every time we log something!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2020-04-21 16:28:42 UTC
Created attachment 103393 [details] [review]
Bug 25172: Adjust tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2020-04-21 16:28:45 UTC
Created attachment 103394 [details] [review]
Bug 25172: Identify and display possible problems on the about page

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2020-04-21 16:28:48 UTC
Created attachment 103395 [details] [review]
Bug 25172: Add missing filter

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Martin Renvoize 2020-04-22 12:44:47 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 28 Jonathan Druart 2020-04-22 14:41:15 UTC
Created attachment 103485 [details] [review]
Bug 25172: Fix Auth_with_ldap.t

#   Failed test 'checkpw_ldap tests'
  #   at t/db_dependent/Auth_with_ldap.t line 324.
  {UNKNOWN}: Configuration not defined at /usr/share/perl5/Log/Log4perl/Config.pm line 579. at /kohadevbox/koha/Koha/Patron.pm line 395
Comment 29 Martin Renvoize 2020-04-22 14:43:32 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 30 Katrin Fischer 2020-04-25 23:18:32 UTC
Could bug 25284 be related to this?
Comment 31 Katrin Fischer 2020-04-25 23:23:27 UTC
(In reply to Katrin Fischer from comment #30)
> Could bug 25284 be related to this?

Testing strongly suggest it does... linking.
Comment 32 Joy Nelson 2020-05-08 19:44:11 UTC
not backported to 19.11.x