Bug 34653 - Make koha-foreach return the correct status code
Summary: Make koha-foreach return the correct status code
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Evan Giles
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-30 03:47 UTC by Evan Giles
Modified: 2023-10-17 11:45 UTC (History)
7 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:
23.11.00,23.05.04,22.11.11


Attachments
Fix te exit code for koha-foreach (1.21 KB, patch)
2023-09-02 03:34 UTC, Evan Giles
Details | Diff | Splinter Review
Fix to exit code for koha-foreach (1.71 KB, patch)
2023-09-06 02:27 UTC, Evan Giles
Details | Diff | Splinter Review
Bug 34653: Make koha-foreach return the correct status code (1.71 KB, patch)
2023-09-17 23:59 UTC, Evan Giles
Details | Diff | Splinter Review
Bug 34653: Make koha-foreach return the correct status code (1.76 KB, patch)
2023-09-18 03:08 UTC, David Nind
Details | Diff | Splinter Review
Bug 34653: Make koha-foreach return the correct status code (1.86 KB, patch)
2023-09-22 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Evan Giles 2023-08-30 03:47:29 UTC
Hi 

Related to #33645 (and probably #25634), I notice that koha-foreach always returns 0 (success) even if the commands it tried to run failed.

I think the correct behavior for this script should be that koha-foreach will return success if all the commands it tried to run succeeded, but failure if any of the commands failed.

Test plan:
0) `koha-foreach /bin/true; echo $?`
1) Note that the exit status is 0
2) `koha-foreach /bin/false; echo $?`
3) Note that the exit status is 0 (incorrect)

4) Apply patch

5) `koha-foreach /bin/true; echo $?`
6) Note that the exit status is 0
7) `koha-foreach /bin/false; echo $?`
8) Note that the exit status is 1 (correct)
Comment 1 Evan Giles 2023-09-02 03:34:31 UTC
Created attachment 155160 [details] [review]
Fix te exit code for koha-foreach
Comment 2 David Cook 2023-09-04 00:07:29 UTC
It would be good to test this with a command that works for the first instance but not the next instance.

If I call correctly "set -e" should cause the whole koha-foreach to fail at that point so in theory this patch should work...
Comment 3 David Cook 2023-09-05 23:54:27 UTC
The test for this one is a bit too simplistic, and it misses some steps.

For instance, after "Apply patch", you need to either copy debian/scripts/koha-foreach to /usr/sbin/koha-foreach, or run the command from "debian/scripts/koha-foreach".

As for the simplicity, I've created an alternative test:

1. koha-create --create-db test
2. vi test.sh
#!/bin/sh
if [ $USER = 'kohadev-koha' ]; then
    echo "FAILED";
    exit 1;
else
    echo "SUCCESS";
    exit 0;
fi

root@kohadevbox:koha(bug_34653)$ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
root@kohadevbox:koha(bug_34653)$ echo $?
1

--

With this patch, the exit code will be whatever the last non-zero exit code was in the koha-foreach loop. I'm not sure that's really what we want. 

I think it might be better if we exit with "1" if any command has a non-zero exit code. 

To do that, we could keep the current patch for the most part, or we could increment an error counter, or something else.

Because we're running in a loop and apparently koha-foreach doesn't ditch out on  non-zero exit code, I don't think we should just return the last $rv encountered.
Comment 4 David Cook 2023-09-05 23:55:00 UTC
(Little reminder to change the Assignee to yourself after posting a patch as well.)
Comment 5 Evan Giles 2023-09-06 00:03:03 UTC
Thanks David

That's all very helpful feedback.

I think understand everything you're saying, so I'll update the patch to include all that.  We'll find out if I truly understand soon :)

Thanks again
Comment 6 Evan Giles 2023-09-06 02:27:43 UTC
Created attachment 155248 [details] [review]
Fix to exit code for koha-foreach
Comment 7 Evan Giles 2023-09-12 02:30:25 UTC
Oh, I realise i need to update the status now...

Please let me know if there is anything else required.

Thanks
Comment 8 David Nind 2023-09-16 22:02:38 UTC
Hi Evan.

This seems to work - using the test plan David Cook provided in comment #3.

I haven't signed this off, as the patch needs to be formatted as per the Commit messages guideline - see  https://wiki.koha-community.org/wiki/Commit_messages

David (Nind)
Comment 9 Evan Giles 2023-09-17 23:59:20 UTC
Created attachment 155770 [details] [review]
Bug 34653: Make koha-foreach return the correct status code

I think the correct behavior for this script should be that koha-foreach
will return 0 (success) if all the commands it tried to run succeeded, but
1 (failure) if any of the commands failed.

To test:
1. $ koha-create --create-db test
2. $ vi test.sh
if [ $USER = 'kohadev-koha' ]; then
    echo "FAILED";
    exit 1;
else
    echo "SUCCESS";
    exit 0;
fi

3. $ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
4. $ echo $?
5. Note that the exit status is 0 (success)

6. Apply patch

7. $ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
8. $ echo $?
9. Note that the exit status is 1 (failure)
Comment 10 Evan Giles 2023-09-18 00:08:02 UTC
Hi David

I did see the "Commit Messages" wiki page, and tried to follow the guidelines there.  Apologies for being difficult, but could you please let me know which conventions I missed in the commit message?  It's not obvious to me as a beginner..

I don't know if it helps, but I have just installed & configured "git bz" to re-upload the patch.  I see that it's done some things differently (maybe better?)

Any advice welcome!
Thanks
Comment 11 David Nind 2023-09-18 03:08:18 UTC
Created attachment 155777 [details] [review]
Bug 34653: Make koha-foreach return the correct status code

I think the correct behavior for this script should be that koha-foreach
will return 0 (success) if all the commands it tried to run succeeded, but
1 (failure) if any of the commands failed.

To test:
1. $ koha-create --create-db test
2. $ vi test.sh
if [ $USER = 'kohadev-koha' ]; then
    echo "FAILED";
    exit 1;
else
    echo "SUCCESS";
    exit 0;
fi

3. $ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
4. $ echo $?
5. Note that the exit status is 0 (success)

6. Apply patch

7. $ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
8. $ echo $?
9. Note that the exit status is 1 (failure)

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2023-09-18 03:12:41 UTC
(In reply to Evan Giles from comment #10)
> Hi David
> 
> I did see the "Commit Messages" wiki page, and tried to follow the
> guidelines there.  Apologies for being difficult, but could you please let
> me know which conventions I missed in the commit message?  It's not obvious
> to me as a beginner..
> 
> I don't know if it helps, but I have just installed & configured "git bz" to
> re-upload the patch.  I see that it's done some things differently (maybe
> better?)
> 
> Any advice welcome!
> Thanks

Hi Evan.

It looks like it was my mistake - I was expecting to see the bug title, description and test plan as part of comment #6. Looking at the details for the obsoleted patches, it looks like it was there all along!

I'm not sure, but I guess it must depend on how the patch is attached to the bug.

I normally (using koha-testing-docker) I go:
- git so X
- git bz attach -e bugnumber HEAD~X..
(where X = the number of patches)

It all looks good now, and I've signed off.

Thanks!

David
Comment 13 Marcel de Rooy 2023-09-22 09:09:45 UTC
Created attachment 156042 [details] [review]
Bug 34653: Make koha-foreach return the correct status code

I think the correct behavior for this script should be that koha-foreach
will return 0 (success) if all the commands it tried to run succeeded, but
1 (failure) if any of the commands failed.

To test:
1. $ koha-create --create-db test
2. $ vi test.sh
if [ $USER = 'kohadev-koha' ]; then
    echo "FAILED";
    exit 1;
else
    echo "SUCCESS";
    exit 0;
fi

3. $ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
4. $ echo $?
5. Note that the exit status is 0 (success)

6. Apply patch

7. $ debian/scripts/koha-foreach sh test.sh
FAILED
kohadev: 1 status returned by "sh test.sh"
SUCCESS
8. $ echo $?
9. Note that the exit status is 1 (failure)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Tomás Cohen Arazi 2023-09-25 13:56:59 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 15 Fridolin Somers 2023-09-25 19:39:01 UTC
Pushed to 23.05.x for 23.05.04
Comment 16 Jacob O'Mara 2023-10-17 11:45:14 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x