Bug 25634 - koha-foreach exits too early if any command has non-zero status
Summary: koha-foreach exits too early if any command has non-zero status
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on: 17532
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-01 05:13 UTC by David Cook
Modified: 2021-12-13 21:09 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.01, 19.11.07, 19.05.12


Attachments
Bug 25634: Warn if koha-shell returns non-zero in koha-foreach (1.55 KB, patch)
2020-06-01 05:55 UTC, David Cook
Details | Diff | Splinter Review
Bug 25634: [Alternate] Warn if koha-shell returns non-zero in koha-foreach (1.61 KB, patch)
2020-06-01 23:53 UTC, David Cook
Details | Diff | Splinter Review
Bug 25634: [Alternate] Warn if koha-shell returns non-zero in koha-foreach (1.68 KB, patch)
2020-06-08 11:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25634: [Alternate] Warn if koha-shell returns non-zero in koha-foreach (1.73 KB, patch)
2020-06-08 14:20 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-06-01 05:13:18 UTC
In 19.11.05 (and certainly other versions affected by Bug 17532), koha-foreach will exit early if 1 command for 1 instance has a non-zero status.

It seems to me that we should warn on a failure, but we should continue the for loop, because the failure could be instance (rather than system) specific. 

In my case, /usr/share/koha/bin/cronjobs/overdue_notices.pl isn't running for most instances, because one of the instances starting with A is encountering this error: "No branches with active overduerules at /usr/share/koha/bin/cronjobs/overdue_notices.pl line 347".

While I'll address that individually, it's an unfortunate consequence of Bug 17532 to ditch out early.

I'm thinking the easiest solution would be to remove "set -e" from koha-foreach, and warn on individual failures.
Comment 1 David Cook 2020-06-01 05:33:35 UTC
I suppose there might be times where you want a koha-foreach to die if any command fails, but perhaps we should add a flag to koha-foreach for that scenario...
Comment 2 David Cook 2020-06-01 05:55:56 UTC
Created attachment 105453 [details] [review]
Bug 25634: Warn if koha-shell returns non-zero in koha-foreach

By putting koha-shell in an if statement, "set -e" will no longer
cause the entire koha-foreach program to exit, if koha-shell
returns a non-zero status.

If a non-zero status is returned, we warn on it, and the caller
of koha-foreach can interpret that command visually.

To Test:
1) Write a shell script that says "Hello" and then exits with a 1
status
2) Run koha-foreach against that shell script with multiple instances
available to koha-foreach
3) Before the patch, koha-foreach will die after the first "Hello"
4) After the patch, koha-foreach will continue, through all the
instances, and report which instances returned non-zero statuses
by which command.
Comment 3 David Cook 2020-06-01 05:57:36 UTC
(In reply to David Cook from comment #0)
> I'm thinking the easiest solution would be to remove "set -e" from
> koha-foreach, and warn on individual failures.

Actually, the easiest solution was to wrap koha-shell in an if statement ad just warn on what instance and command return a non-zero status.

"set -e" won't exit if the non-zero status is returned by a command wrapped by an if statement.
Comment 4 Jonathan Druart 2020-06-01 08:03:23 UTC
I agree, should not we display the error code?
Comment 5 David Cook 2020-06-01 23:50:05 UTC
(In reply to Jonathan Druart from comment #4)
> I agree, should not we display the error code?

Mmm good point.

I'll add an alternative patch.
Comment 6 David Cook 2020-06-01 23:53:46 UTC
Created attachment 105478 [details] [review]
Bug 25634: [Alternate] Warn if koha-shell returns non-zero in koha-foreach

By putting koha-shell in an if statement, "set -e" will no longer
cause the entire koha-foreach program to exit, if koha-shell
returns a non-zero status.

If a non-zero status is returned, we warn on it, and the caller
of koha-foreach can interpret that command visually.

To Test:
1) Write a shell script that says "Hello" and then exits with a 1
status
2) Run koha-foreach against that shell script with multiple instances
available to koha-foreach
3) Before the patch, koha-foreach will die after the first "Hello"
4) After the patch, koha-foreach will continue, through all the
instances, and report which instances returned non-zero statuses
by which command.
Comment 7 Kyle M Hall 2020-06-08 11:08:04 UTC
Created attachment 105636 [details] [review]
Bug 25634: [Alternate] Warn if koha-shell returns non-zero in koha-foreach

By putting koha-shell in an if statement, "set -e" will no longer
cause the entire koha-foreach program to exit, if koha-shell
returns a non-zero status.

If a non-zero status is returned, we warn on it, and the caller
of koha-foreach can interpret that command visually.

To Test:
1) Write a shell script that says "Hello" and then exits with a 1
status
2) Run koha-foreach against that shell script with multiple instances
available to koha-foreach
3) Before the patch, koha-foreach will die after the first "Hello"
4) After the patch, koha-foreach will continue, through all the
instances, and report which instances returned non-zero statuses
by which command.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Martin Renvoize 2020-06-08 14:20:59 UTC
Created attachment 105648 [details] [review]
Bug 25634: [Alternate] Warn if koha-shell returns non-zero in koha-foreach

By putting koha-shell in an if statement, "set -e" will no longer
cause the entire koha-foreach program to exit, if koha-shell
returns a non-zero status.

If a non-zero status is returned, we warn on it, and the caller
of koha-foreach can interpret that command visually.

To Test:
1) Write a shell script that says "Hello" and then exits with a 1
status
2) Run koha-foreach against that shell script with multiple instances
available to koha-foreach
3) Before the patch, koha-foreach will die after the first "Hello"
4) After the patch, koha-foreach will continue, through all the
instances, and report which instances returned non-zero statuses
by which command.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-06-08 14:21:22 UTC
Works as expected, thanks.

Passing QA
Comment 10 Jonathan Druart 2020-06-12 12:17:40 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Lucas Gass 2020-06-17 20:34:53 UTC
backported to 20.05.x for 20.05.01
Comment 12 Aleisha Amohia 2020-06-17 21:27:46 UTC
backported to 19.11.x for 19.11.07
Comment 13 Victor Grousset/tuxayo 2020-06-18 16:51:47 UTC
Backported to 19.05.x branch for 19.05.12