Improve 'bin/run-all-petstore'

[jmini]

Fix for #132.

[jmini]

This should fail, because bin/jaxrs-cxf-cdi-petstore-server.sh is broken on this branch (script is fixed with #131)

[jimschubert]

This is already what the -e on the first line of this file does.

[jimschubert]

@jmini I've added a fix to the latest commit of #57 (ec3cd97) to exit with status code 1 when the CLI is run without args. When run with invalid args, it already throws a parser exception.

Adding an exit code internally to the command would cause issues if someone wanted to pipe the output of this default command to tr or something else. For instance, in #57, I've removed the Available x: prefix and output only a csv of generator names when --short is included. This works similarly to many popular tools (git, docker, and others).

I think run-all-petstore should also run all generators, regardless of one or more failures. This way we can accumulate and fix all failures on CI, rather than failing on the first one, fixing it and finding there may be other failures that don't repro locally.

Here's an example:

#!/bin/bash
# this bash script will loop through all the .sh files under bin
# execute the script and check the result (exit code) to see if
# there's any error

echo "IMPORTANT: this script should be run by the CI (e.g. Shippable) only. There's no need to run this script to update Petstore samples for all generators."
echo "Please press CTRL+C to stop or the script will continue in 10 seconds."

sleep 10

successes=0
failures=0
for SCRIPT in $(ls -l ./bin/*.sh | grep -v all)
do
    if [ -f ${SCRIPT} -a -x ${SCRIPT} ]; then
        echo "Running $SCRIPT"
        ${SCRIPT}
        rc=$?
        if [[ ${rc} != 0 ]]; then
            >&2 echo "ERROR!! FAILED TO RUN ${SCRIPT}"
            ((failures+=1))
        else
            ((successes+=1))
        fi
    fi
done

if (( failures > 0 )); then
   >&2 echo "[ERROR] ${failures} out of $((failures+successes)) scripts failed."
   exit 1
else
    echo "[SUCCESS] ${successes} generators finished."
fi

This script will run all generators. If one or more failed, you'd receive an exit code of 1 and an error like

[ERROR] 1 out of 80 scripts failed.

If it succeeded it would output a friendly message like

[SUCCESS] 80 generators finished.

I did some testing with the current run-all-petstore and because of the -e opt set on the first line of the file, the script already fails hard and skips the existing error message. If you're not familiar with the option after the shebang, bash (and other commands) can be passed a single option (or more on some systems). This is documented in the execve manpage for your system. On OS X, this says it supports varargs, but I believe it is POSIX standard to only support a single arg. For bash, this argument can be a command line argument (man bash) or a shell option value. You can inspect all available shell options with help set.

Because this PR is targeting a fix for #132. I was wondering if you'd mind evaluating the script I have above to see what you think about usability and CI visibility to successes/failures? We may have differing concerns about running this script (e.g. fail fast if it's a "build script", aggregate concerns if it's an "integration test").

[jmini]

In this build we have the notification that a build that is failing (this is what we want, be notified by the CI when something is broken)

Log in Shippable job 323:

[main] INFO org.openapitools.codegen.AbstractGenerator - writing file /root/src/github.com/OpenAPITools/openapi-generator/samples/server/petstore/java-vertx/rx/.openapi-generator/VERSION
./bin/jaxrs-cxf-cdi-petstore-server.sh: 30: ./bin/jaxrs-cxf-cdi-petstore-server.sh: =generate -t modules/openapi-generator/src/main/resources/JavaJaxRS/cxf-cdi -i modules/openapi-generator/src/test/resources/2_0/petstore.yaml -l jaxrs-cxf-cdi -o samples/server/petstore/jaxrs-cxf-cdi -DhideGenerationTimestamp=true : not found
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=256M; support was removed in 8.0

[jmini]

You seems to be much better bash developer than I am... Sorry I missed your second response in my first one.

Are you sure that this is working:

        echo "Running $SCRIPT"
        ${SCRIPT}
        rc=$?

Or do we need to add something in each script to propagate back the error status sent by the java command to be the result of the script itself?

[jmini]

Yes the Add System.exit(1) to Langs cmd needs to be removed from this PR. It is covered by #57.

[jimschubert]

@jmini that's weird. I tested with run-all-petstore and the -e argument on the shebang line and scripts failed fast for me. I wonder if some script being called is causing unexpected behavior or if there are difference between bash versions (mine, yours, and Shippable).

Per bash documentation (https://www.gnu.org/software/bash/manual/bashref.html#Invoking-Bash):

All of the single-character options used with the set builtin (see The Set Builtin) can be used as options when the shell is invoked.

manpage on OS X reads:

In addition to the single-character shell options documented in the description of the set builtin command, bash interprets the following options when it is invoked

However, when I test with minimal implementations outside of run-all-petstore, the files don't immediately exit when a called file exposes a non-zero exit code:

$ cat first
#!/bin/bash -e
sh second
echo "Result $?"

$ cat second
#!/bin/sh
exit 1

$ sh first
Result 1

I personally create bash scripts with a structure of:

#!/usr/bin/env bash
set -euxo pipefail
…

But, I wonder why the shebang and bash arguments don't work as document by bash? I'm sure whoever originally wrote this script believed the -e worked as it does with set. I seem to recall this is how it worked 10-15 years ago, but I could be wrong.

[jmini]

Thank you a lot of this investigations.

---

I now have pushed your version (where we do not want to use -e at all to collect the results). I put you as author of the commit.

Let see how it works.

---

Then I will push a test commit, where I break again the bin/jaxrs-cxf-cdi-petstore-server.sh script to see if the CI catch it.

[jimschubert]

@jmini I don't know if you're interested, but I've found reasoning why the -e has inconsistent behavior at https://kvz.io/blog/2013/11/21/bash-best-practices/:

Avoid using #!/usr/bin/env bash -e (vs set -e), because when someone runs your script as bash ./script.sh, the exit on error will be ignored.

This would explain why this is different via CI, and could explain why you and I have different experiences.

I also found on TLDP that -e can be ignored in looping constructs.

[jmini]

Test for commit d578369

Shippable — Run 328 status is SUCCESS

I see this in the log:

[SUCCESS] 177 generators finished.

[jmini]

I now have pushed a wrong commit b5c30f9, with this commit Shippable CI shoud fail with an error message.

PLEASE DO NOT MERGE THIS PR NOW

[jmini]

It works as expected for b5c30f9:

Shippable — Run 331 status is FAILED.

I see in the log:

[ERROR] 1 out of 177 scripts failed.

[jmini]

After f90f9ab :

Shippable — Run 334 status is SUCCESS.

This looks good to me. I think we can implement it like this. Thank you a lot @jimschubert

[jimschubert]

I'm on mobile atm, and can't merge due to CircleCI failure. Looks like this can be merged now, it someone else wants to merge it. If not, I'll get it done by this evening.

[jmini]

ci/circleci — Your tests failed on CircleCI, Logs:

[info] 	[SUCCESSFUL ] jline#jline;2.12.1!jline.jar (332ms)
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 	::              FAILED DOWNLOADS            ::
[warn] 	:: ^ see resolution messages for details  ^ ::
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 	:: joda-time#joda-time;2.2!joda-time.jar
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
sbt.ResolveException: download failed: joda-time#joda-time;2.2!joda-time.jar