-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do more with ShellCheck #254
Conversation
* Fix numerous issues to make ShellCheck pass across `bin/*` and `conf/uno.conf` * Add GitHub Actions and CI script for automated ShellCheck testing * Add missing checksum for latest Hadoop 3.1, since it's still supported by Accumulo * Fix parameter passing for `--no-deps` flag * Build SNAPSHOT version with correct version of Guava for Hadoop 3.1 and later * Make some long if statements into more concise `&&` syntax for readability * Remove some unnecessary quoting * Rely on return code for pgrep for many statements that were previously relying on its output unnecessarily
This isn't quite ready for merging. I saw a bug where this didn't properly trigger the install of ZooKeeper after installing Hadoop, when doing |
Consolidate some of the scripts into a commands.sh file and use return statements instead of exit statements, as appropriate, for more reliable exit behavior for functions.
@keith-turner I fixed the previous error, if you want to take another pass at a review. Some of the conditional statements in bash at the end of some scripts were returning I fixed this by adding a 'true' statement at the end of called scripts, so that they don't return false, if they aren't supposed to. In order to track this down, I ended up consolidating a few smaller scripts into a single script with multiple functions. This refactor helped me track down the bug fix the problem. I'm leaving it included in this PR, because I think it's better this way, and using functions instead of scripts helps with understanding return codes. I think there's more opportunities to clean up additional things in these scripts, but this PR is already too big, so I'll defer to another time. |
exit 1 | ||
esac | ||
|
||
# fetch.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of these comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mostly style. They help me remember which file I'm editing, and help provide a non-empty final line for the script, so that I don't get warnings with git add --patch
. But, they serve no real functional purpose.
|
||
case "$uno_cmd" in | ||
ashell|env|fetch|install|kill|run|setup|start|status|stop|version|wipe|zk) | ||
"uno_${uno_cmd}_main" "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice refactoring. Its a bit hard to review, I am just going to trust that you copied the code from the files to functions correctly.
Instead of having the switch statement enumerate all the command names, wonder if it would work to check if the function exists instead. Maybe something like the following, although I am not sure if it would even work.
if [ "$(type -t uno_${uno_cmd}_main)" = 'function' ]; then
"uno_${uno_cmd}_main" "$@"
else
uno_help_main "$@"
fi
Below is a post I found about checking if a function exists in bash to write the above.
https://stackoverflow.com/questions/1007538/check-if-a-function-exists-from-a-bash-script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, and thought about doing something like this, but didn't look into it to figure out the syntax. One thing I wasn't sure about was compatibility with Bash 3. I tried to avoid any changes I thought might not work on Bash 3. Since I don't have a macOS machine to test on, I chose to avoid this one because of uncertainty of whether it would work there.
bin/*
andconf/uno.conf
by Accumulo
--no-deps
flagand later
&&
syntax forreadability
relying on its output unnecessarily