CI does not properly handle lint errors

[ptitzler]

Describe the issue
The lint-server github action https://github.com/elyra-ai/elyra/blob/main/.github/workflows/build.yml#L46-L52 does not terminate as expected if flake8 raises an error, as seen here and all subsequent CI runs.

Note that flake8 returns a non-zero status code, yet the next commands (echo + black) in the Makefile are executed:

Since the last command (python -m black ...) returns a zero return code, the action's outcome is interpreted as success.

make lint-server behaves as expected in a local build environment:

$ make lint-server
python3 -m flake8 elyra .github
elyra/tests/pipeline/test_handlers.py:33:1: I100 Import statements are in the wrong order. 'from elyra.pipeline.processor import PipelineProcessorManager' should be before 'from elyra.tests.util.handlers_utils import expected_http_error'
elyra/tests/pipeline/test_handlers.py:33:1: I202 Additional newline in a group of imports. 'from elyra.pipeline.processor import PipelineProcessorManager' is identified as Application and 'from elyra.tests.util.handlers_utils import expected_http_error' is identified as Application.
elyra/tests/pipeline/test_handlers.py:36:1: I100 Import statements are in the wrong order. 'from elyra.pipeline.parser import PipelineParser' should be before 'from elyra.pipeline.validation import ValidationResponse'
make: *** [lint-server] Error 1

[kevin-bates]

This is really interesting and implies any "multi-step" make targets could ignore failures in interim steps!

I couldn't easily determine which version of make is used in GH actions so I added a temporary CI action to dump the version and help information from make. Here's the output (fyi), but its essentially the same make we document to use: GNU make - although a much more recent version that what I'm using in my dev. I'll upgrade to this version, but I suspect there's something else in play here.

> Run make --version
GNU Make 4.2.1
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

> Run make --help
Usage: make [options] [target] ...
Options:
  -b, -m                      Ignored for compatibility.
  -B, --always-make           Unconditionally make all targets.
  -C DIRECTORY, --directory=DIRECTORY
                              Change to DIRECTORY before doing anything.
  -d                          Print lots of debugging information.
  --debug[=FLAGS]             Print various types of debugging information.
  -e, --environment-overrides
                              Environment variables override makefiles.
  --eval=STRING               Evaluate STRING as a makefile statement.
  -f FILE, --file=FILE, --makefile=FILE
                              Read FILE as a makefile.
  -h, --help                  Print this message and exit.
  -i, --ignore-errors         Ignore errors from recipes.
  -I DIRECTORY, --include-dir=DIRECTORY
                              Search DIRECTORY for included makefiles.
  -j [N], --jobs[=N]          Allow N jobs at once; infinite jobs with no arg.
  -k, --keep-going            Keep going when some targets can't be made.
  -l [N], --load-average[=N], --max-load[=N]
                              Don't start multiple jobs unless load is below N.
  -L, --check-symlink-times   Use the latest mtime between symlinks and target.
  -n, --just-print, --dry-run, --recon
                              Don't actually run any recipe; just print them.
  -o FILE, --old-file=FILE, --assume-old=FILE
                              Consider FILE to be very old and don't remake it.
  -O[TYPE], --output-sync[=TYPE]
                              Synchronize output of parallel jobs by TYPE.
  -p, --print-data-base       Print make's internal database.
  -q, --question              Run no recipe; exit status says if up to date.
  -r, --no-builtin-rules      Disable the built-in implicit rules.
  -R, --no-builtin-variables  Disable the built-in variable settings.
  -s, --silent, --quiet       Don't echo recipes.
  -S, --no-keep-going, --stop
                              Turns off -k.
  -t, --touch                 Touch targets instead of remaking them.
  --trace                     Print tracing information.
  -v, --version               Print the version number of make and exit.
  -w, --print-directory       Print the current directory.
  --no-print-directory        Turn off -w, even if it was turned on implicitly.
  -W FILE, --what-if=FILE, --new-file=FILE, --assume-new=FILE
                              Consider FILE to be infinitely new.
  --warn-undefined-variables  Warn when an undefined variable is referenced.

This program built for x86_64-pc-linux-gnu
Report bugs to <bug-make@gnu.org>

[kevin-bates]

This appears to be a make version issue. If I use the latest make:

$ /usr/local/Cellar/make/4.3/bin/gmake -v         
GNU Make 4.3
Built for x86_64-apple-darwin21.1.0
Copyright (C) 1988-2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

then run it directly against the lint-server target, I get the following:

$ /usr/local/Cellar/make/4.3/bin/gmake lint-server
flake8 elyra .github
echo python3 -m black --check --diff --color .
python3 -m black --check --diff --color . || (echo "Black formatting encountered issues.  Use 'make black-format' to apply the suggested changes."; exit 1)
elyra/tests/pipeline/test_handlers.py:33:1: I100 Import statements are in the wrong order. 'from elyra.pipeline.processor import PipelineProcessorManager' should be before 'from elyra.tests.util.handlers_utils import expected_http_error'
elyra/tests/pipeline/test_handlers.py:33:1: I202 Additional newline in a group of imports. 'from elyra.pipeline.processor import PipelineProcessorManager' is identified as Application and 'from elyra.tests.util.handlers_utils import expected_http_error' is identified as Application.
elyra/tests/pipeline/test_handlers.py:36:1: I100 Import statements are in the wrong order. 'from elyra.pipeline.parser import PipelineParser' should be before 'from elyra.pipeline.validation import ValidationResponse'
python3 -m black --check --diff --color .
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install black[jupyter]``
All done! ✨ 🍰 ✨
141 files would be left unchanged.

So we know make >= 4.2.1 exhibit this behavior, while GNU make == 3.81 does not. I suspect this make be a version 4.0 change (sigh).

[kevin-bates]

Looks like the .ONESHELL directive is the culprit: https://github.com/kevin-bates/elyra/runs/7862535554?check_suite_focus=true

When removed, I get the expected behavior with more recent versions of make.