Bug 17532 - koha-shell -c does not propagate the error code
Summary: koha-shell -c does not propagate the error code
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25635 25634
  Show dependency treegraph
 
Reported: 2016-11-01 15:57 UTC by Jonathan Druart
Modified: 2020-11-30 21:47 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Before this development, the koha-shell script would always return a successful error code, making it hard for the callers to notice there was a problem with the command being run inside the instance's shell. This development makes koha-shell propagate the running scripts' error code so the caller can take the required actions. Note: this implies a behaviour change (for good) but a warning should be added to the release notes. Right now it always returns
Version(s) released in:
20.05.00, 19.11.05, 19.05.10


Attachments
Bug 17532: Make koha-shell -c propagate the error code (1.33 KB, patch)
2016-11-01 16:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17532: Make koha-shell propagate the error code (1.35 KB, patch)
2020-02-13 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17532: Make koha-shell propagate the error code (1.55 KB, patch)
2020-03-02 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17532: Make koha-shell propagate the error code (1.61 KB, patch)
2020-03-02 15:31 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 Jonathan Druart 2016-11-01 15:57:54 UTC

    
Comment 1 Jonathan Druart 2016-11-01 16:23:07 UTC
Created attachment 57055 [details] [review]
Bug 17532: Make koha-shell -c propagate the error code

When you execute a command using koha-shell -c, you would like to get
the error code of the command you executed.
It seems that koha-shell does not do that.
Comment 2 Jonathan Druart 2016-11-01 16:23:39 UTC
Not sure about this, in my last tests it worked as expected.
Comment 3 Jonathan Druart 2020-02-13 10:57:14 UTC
Reviving this one.
Comment 4 Jonathan Druart 2020-02-13 11:03:23 UTC
Context: I am trying to make koha-misc4dev stop when something wrong happens. Basically we do not want the process to continue of the DB has not been populated correctly.
It will make the errors easier to catch.

The issue: Say a script that will return an error (die in perl for an easy example), koha-shell will not return the 255 error code, but 0 instead. The caller cannot know something wrong happened

The solution is to propagate the error and make koha-shell return the same error code as the command it executed

An example:
=== t.pl ===
die('something wrong');

% perl t.pl;echo $?
something wrong at t.pl line 1.
255

% sudo koha-shell kohadev -p -c 'perl xxx.pl' ; echo $?
something wrong at xxx.pl line 1.
0
Comment 5 Jonathan Druart 2020-02-13 11:05:02 UTC
Created attachment 98808 [details] [review]
Bug 17532: Make koha-shell propagate the error code

Context: I am trying to make koha-misc4dev stop when something wrong happens.
Basically we do not want the process to continue of the DB has not been populated correctly.
It will make the errors easier to catch.

The issue: Say a script that will return an error (die in perl for an easy example),
koha-shell will not return the 255 error code, but 0 instead.
The caller cannot know something wrong happened

The solution is to propagate the error and make koha-shell return the same error code
as the command it executed

An example:
=== t.pl ===
die('something wrong');

% perl t.pl;echo $?
something wrong at t.pl line 1.
255

% sudo koha-shell kohadev -p -c 'perl xxx.pl' ; echo $?
something wrong at xxx.pl line 1.
0
Comment 6 Jonathan Druart 2020-02-13 11:08:55 UTC
Do you see any issues this change could cause?
Comment 7 Martin Renvoize 2020-03-02 11:20:19 UTC
This looks pretty sensible and sane to me
Comment 8 Tomás Cohen Arazi 2020-03-02 14:20:28 UTC
Created attachment 99918 [details] [review]
Bug 17532: Make koha-shell propagate the error code

Context: I am trying to make koha-misc4dev stop when something wrong happens.
Basically we do not want the process to continue of the DB has not been populated correctly.
It will make the errors easier to catch.

The issue: Say a script that will return an error (die in perl for an easy example),
koha-shell will not return the 255 error code, but 0 instead.
The caller cannot know something wrong happened

The solution is to propagate the error and make koha-shell return the same error code
as the command it executed

An example:
=== t.pl ===
die('something wrong');

% perl t.pl;echo $?
something wrong at t.pl line 1.
255

% sudo koha-shell kohadev -p -c 'perl xxx.pl' ; echo $?
something wrong at xxx.pl line 1.
0

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tested it with
  $ sudo debian/scripts/koha-shell kohadev -c 'exit 8'
  $ echo $?
  8
The unpatched version returns 0 (forgets about the error code).
Comment 9 Jonathan Druart 2020-03-02 14:25:04 UTC
I should note:
I have no idea how widely this script is used, but that will certainly introduced regressions for people using it.
For Jenkins that will stop the build if something wrong happens during the insert of the data in the DB (expected behavior).
Comment 10 Tomás Cohen Arazi 2020-03-02 14:29:18 UTC
(In reply to Jonathan Druart from comment #9)
> I should note:
> I have no idea how widely this script is used, but that will certainly
> introduced regressions for people using it.
> For Jenkins that will stop the build if something wrong happens during the
> insert of the data in the DB (expected behavior).

I think it is worth changing the (currently) wrong behaviour. It will even help sysadmins using it detect problems. We should of course mention this on the release notes.

Good catch (as usual), Jonathan.
Comment 11 Jonathan Druart 2020-03-02 14:49:45 UTC
s/regressions/side-effects
;)
Comment 12 Martin Renvoize 2020-03-02 15:31:47 UTC
Created attachment 99943 [details] [review]
Bug 17532: Make koha-shell propagate the error code

Context: I am trying to make koha-misc4dev stop when something wrong happens.
Basically we do not want the process to continue of the DB has not been populated correctly.
It will make the errors easier to catch.

The issue: Say a script that will return an error (die in perl for an easy example),
koha-shell will not return the 255 error code, but 0 instead.
The caller cannot know something wrong happened

The solution is to propagate the error and make koha-shell return the same error code
as the command it executed

An example:
=== t.pl ===
die('something wrong');

% perl t.pl;echo $?
something wrong at t.pl line 1.
255

% sudo koha-shell kohadev -p -c 'perl xxx.pl' ; echo $?
something wrong at xxx.pl line 1.
0

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tested it with
  $ sudo debian/scripts/koha-shell kohadev -c 'exit 8'
  $ echo $?
  8
The unpatched version returns 0 (forgets about the error code).

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-03-03 09:22:59 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-04-01 21:20:36 UTC
Backported to 19.11.x for 19.11.05
Comment 15 Lucas Gass 2020-04-09 19:37:59 UTC
backported to 19.05.x for 19.05.10
Comment 16 Hayley Pelham 2020-04-14 21:21:14 UTC
Enhancement will not be backported to 18.11.x series
Comment 17 David Cook 2020-06-01 05:07:06 UTC
This has a perhaps unforeseen consequence with koha-foreach.

Since koha-foreach uses "set -e", if 1 instance runs a command with koha-shell which has a non-zero status, koha-foreach exits. If you have 10 instances and the 1st produces an error, the other 9 instances won't have their commands run at all. (https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html#The-Set-Builtin)

I can't imagine that's what we'd want to do? I'll open a new bug report.