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

[WIP] Add ShellCheck to check all .sh, .bash, .bats files #227

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

luck3y
Copy link
Contributor

@luck3y luck3y commented Mar 25, 2021

No description provided.

@luck3y luck3y requested a review from jfdenise March 25, 2021 22:57
@luck3y
Copy link
Contributor Author

luck3y commented Mar 25, 2021

@jfdenise this can wait until after the next release IMO, but we should get it in right after that so we have some time to keep an eye on it. This only addresses the ShellCheck errors, so theres a few more things to look at later.

Test run here: https://github.com/luck3y/wildfly-cekit-modules/actions/runs/688163867

@jfdenise
Copy link
Contributor

@luck3y , this seems very good to me. All these changes have been applied automatically by some tooling? Shouldn't we merge this asap and enable the checker job? BTW, we also have scripts in other repos that coud benefit from this check.

@luck3y
Copy link
Contributor Author

luck3y commented Mar 26, 2021

@jfdenise This is a combination of (for example):

$ find ./ -name "*.sh" | xargs shellcheck -S error -f diff | patch -p1

and some manual changes that shellcheck can't patch automatically.

I agree that it appears fine, the only reason I suggest waiting until the next (minor) release is that it could introduce new issues and thats a slight risk -- but the tests passing suggest that perhaps it would be ok.

I'm fine with either doing it now or waiting, thats why I asked you :)

@luck3y
Copy link
Contributor Author

luck3y commented Mar 26, 2021

and +1 to the other repos, I was planning on doing this for jboss-eap-modules as well. Do we have others?

logger_level=$(echo $logger | awk -F':' '{print $2}')
logger_category=$(echo "$logger" | awk -F':' '{print $1}')
logger_level=$(echo "$logger" | awk -F':' '{print $2}')
allowed_log_levels_str=$(printf ",%s", "${allowed_log_levels[@]}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this is a manual change, converting an array to a string seperated with ','

@@ -94,7 +94,7 @@ EOF
@test "Generate JGroups Auth config - missing cluster password" {
run generate_jgroups_auth_config "" "digest_algo"
echo "Result: ${output}"
[[ "${output}" =~ "No password defined for JGroups cluster." ]]
[[ "${output}" =~ .*"No password defined for JGroups cluster.".* ]]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are changed manually as well to correct the behavior of =~ operating on a regex. The intent here is that the string contains the message, but does not necessarily match the message completly.

@@ -195,8 +194,8 @@ EOF

run configure_jgroups_encryption
echo "${output}"
[[ "${output}" =~ "INFO Configuring JGroups cluster traffic encryption protocol to SYM_ENCRYPT." ]]
[[ "${output}" =~ "WARN Detected missing JGroups encryption configuration, the communication within the cluster WILL NOT be encrypted." ]]
[[ "${output}" =~ .*"INFO Configuring JGroups cluster traffic encryption protocol to SYM_ENCRYPT.".* ]]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same regex comment as above, I won't mention all of these, but something to be aware of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants