fix: insert error handling in list-all & download plugin scripts

[laidbackware]

Summary

This fix will check if a plugin does not exit 0 when calling list-all or download. In the case of an error it will print all stdout and stderr returned by the plugin and then the list all script will exit 1.

Without this fix, it is not possible to test a plugin using CI and get reliable results, as errors are not handled.

Fixes: This fixes #880 and #886

Other Information

This has been tested on Linux and MacOS to be working as expected.

Thank you for contributing to asdf!

[Stratus3D]

Thanks for the PR! I think there are a couple things we need to do before merging this:

• Write tests for list-all failures, and assert that the command works as expected
• When plugin list-all fails, print a line before the plugin output with something along the lines of Plugin <plug name>'s list-all callback script failed with output:

Thanks for the PR! This is a good improvement.

[laidbackware]

Refactored to include tests - which included fixed 2 existing tests.
I broke some affected string tests out to separate lines to aid readability and prevent long lines.
Added check to install script to allow for plugin download to cleanly exit.
The returncode is collected outside the subshell with no default set prior, as if the download/install block doesn't run the return code of the if command will be captured. The script will then tidy up if either download or install do not exit clearnly.

[Stratus3D]

@laidbackware changes look good! I did leave two comments, one about printing failure output to STDERR, and the other is a question about the tests. Let me know if you will address these comments or if you would like me to make the STDERR change before merging.

[laidbackware]

@Stratus3D I have also been thinking about another edge case which I'd be interested on your thoughts on before implementing.

This change is currently written to redirect all STDOUT and STDERR data to a single variable when calling list-all. This is fine if the plugin generates an error, but may cause issues if a plugin uses a tool which happens to write to STDERR under normal operation as some linux tools do.

asdf/lib/commands/command-list-all.bash

Line 12 in e2c4f7c

return_code=0 && versions=$(bash "${plugin_path}/bin/list-all" 2>&1) || return_code=$?

In this case it would skew the output of the asdf command.

The only clean solution I can think of to split the output is to acutally call the list-all script twice. Once just capturing STDOUT which is printed if no errrors occur, but if there is an error it is run a second time to capture STDOUT and STDERR.

What do you think?

[jthegedus]

LGTM. @Stratus3D ?

[laidbackware]

@Stratus3D @jthegedus I think I've solved the issue I mentioned previously without calling the plugin twice. I was concerned that if there were network timeouts, calling the plugin twice could be worse for the user.
Now command-list-all.bash contains a function which splits stdout and stderr into separate variables. When the plugin list-all script exits with code 0, then stderr is ignored and when it exits with a non-zero code stderr then stdout is printed. I've also added tests to ensure stderr is ignored when sucessful and printed when unsuccessful.
All CI tests look good on my end.

[jthegedus]

Thanks very much for contributing this @laidbackware Really appreciate it 😄

[Stratus3D]

Sorry @laidbackware for never responding, looks great :)