Skip to content
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

Merged
merged 7 commits into from
Aug 26, 2020
Merged

Conversation

ctubbsii
Copy link
Member

  • 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

* 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
README.md Show resolved Hide resolved
bin/impl/install.sh Outdated Show resolved Hide resolved
@ctubbsii
Copy link
Member Author

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 uno setup accumulo without doing . <(uno env). You shouldn't need to source the environment for Uno to work properly.. I'm going to have to try to track down the relevant change before I can merge this.

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.
@ctubbsii
Copy link
Member Author

@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 false, which resulted in the return code for the entire script being false, and causing some of the steps to not execute.

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
Copy link
Contributor

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?

Copy link
Member Author

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" "$@"
Copy link
Contributor

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

Copy link
Member Author

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.

@ctubbsii ctubbsii merged commit 76d64de into apache:main Aug 26, 2020
@ctubbsii ctubbsii deleted the shellcheck-improvements branch August 26, 2020 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants