Add stickler shellcheck

[thomasmerz]

By submitting this pull request, I confirm the following:

• I have read and understood the contributors guide, as well as this entire template.
• I have made only one major change in my proposed changes.
• I have commented my proposed changes within the code.
• I have tested my proposed changes, and have included unit tests where possible.
  See https://github.com/thomasmerz/PADD/runs/5036319623?check_suite_focus=true
• I am willing to help maintain this change if there are issues with it later.
• I give this submission freely and claim no ownership.
• It is compatible with the EUPL 1.2 license
• I have squashed any insignificant commits. (git rebase)

Please make sure you Sign Off all commits. Pi-hole enforces the DCO.

---

What does this PR aim to accomplish?:
This PR will introduce a GitHub Action which will run shellcheck on every commit in every branch and also on every PR. The result will show in detail what is not "ok" and advices how to fix it:

How does this PR accomplish the above?:
This PR will use latest shellcheck on every run from the author's repo and let it run in a GitHub Action.

What documentation changes (if any) are needed to support this PR?:
There are no necessary changes.

[thomasmerz]

For a better user and contributors and codeowners experience you could/should add a new Branch protection rule like this:

[yubiuser]

Thanks for this PR. However, it should be made against master branch.

I like the idea of having some kind of style check in this repo. I'm undecided if we should use stickler (as we do in the core repo) or shellcheck directly as you proposed. Having any is better than none - so I'm fine with using this as well.

[yubiuser]

With changing the base branch you changed two more files. I guess this is not intended. I think you need to rebase your local on master as well and do a force push.

[thomasmerz]

👁️ ❤️ force-pushing… 😵‍💫

[DL6ER]

I'd prefer using Stickler mainly for resources but also for user friendliness. The current proposal spawns a Ubuntu VM, downloads the stickler application using wget and runs it afterwards. This needs a lot of resources including quite large downloads (ubuntu-latest is 188 MB alone). Compared to this, stickler runs in a dedicated machine and "just" downloads the single commit to check it. Also, my feeling is that stickler is a lot more user friendly. Compare a shellcheck in a VM that will print errors in the raw actions output to direct code annotations like

[thomasmerz]

@DL6ER , do you agree to use the docker image "koalaman/shellcheck" (or "koalaman/shellcheck-alpine") instead which has only a very, very small size (less than 10 Megabyte)?

What I personally really like about shellcheck are the hints ("did you mean…?") about the warnings and links to the wiki with a more detailled explanation and background info.

✏️ Update: There are only Ubuntu images available - no Alpine Linux at all. And if I understand it right, even Docker runs on-top of these images. See: https://github.com/actions/virtual-environments#available-environments
So I will have a look at this stickler-ci bot…

[thomasmerz]

@DL6ER , I tried stickler because I was indeed very interested in it and I wanted to know how it works and get the "user friendliness" you mentioned. (which really sounds "cool" to have the check results directly in the code, even if I normally edit with vi(m) and not directly in the GitHub GUI…

But I didn't find it user friendly yet, because shellcheck is still complaining about 3 checks and I can't find any details with stickler.
It should also hasn't "All checks … passed" because of some warnings.

Can you please explain where the user/committer can find these infos or what I've done wrong? 🤔 Maybe I'm too much got used with shellcheck, but in this case I find it better under all circumstances.

https://github.com/thomasmerz/PADD/pull/2/checks?check_run_id=5082565314

[DL6ER]

The much smaller base image is already a good start, however, as you found, this may not work. I do understand what you mean by better suggestions (wiki, etc.), but the opposite is that you need to click a few times to really find out why the job failed, once you reached the point, I do agree, the comments are more helpful:

---

It should look like this (where I tool the screenshot above from), when configured correctly:
pi-hole/pi-hole@a0ecfcc

another example: pi-hole/pi-hole@469b715

Maybe you didn't grant Stickler sufficient rights to actually add the comments it wanted to add. It's interesting it didn't fail but it may be that Stickler has to be configured in the target branch (currently, it isn't configured in master).

You should see the comments listed on the stickler action like this when it works:

https://github.com/pi-hole/pi-hole/runs/2976380981

The View more details on Stickler CI will only work for your own PRs and also only for open ones (we currently don't have a open PR with shellcheck errors).

---

Ideally, we can get the best of both worlds, the quoted output above put next to the offending line. So maybe using indeed out own action is necessary to provide the needed flexibility. Then, a good starting point might be https://github.com/marketplace/actions/shellcheck-action

Whatever we'd need to customize in the output format could then be done here.

[thomasmerz]

The much smaller base image is already a good start, however, as you found, this may not work.

Ok, let's concentrate on stickler - I think you really want this 😁

… when configured correctly …
Maybe you didn't grant Stickler sufficient rights to actually add the comments it wanted to add. It's interesting it didn't fail but it may be that Stickler has to be configured in the target branch (currently, it isn't configured in master).

My .stickler.yml in my 'master' looks pretty much the same as yours ans stickler-ci-bot has all permissions that it reuested 🤷🏻‍♂️ And my PR is into my 'master'.

The View more details on Stickler CI

"The repository you were looking for could not be found." 😞

will only work for your own PRs and also only for open ones (we currently don't have a open PR with shellcheck errors).

But we have one in my repo what stickler found out:

🦎🖥  ✔ ~/temp/PRs/PADD [test_stickler_1|…1]
10:45 $ shellcheck padd.sh

In padd.sh line 322:
  if ! [ -z ${PIHOLE_DOMAIN+x} ]; then
     ^-- SC2237: Use [ -n .. ] instead of ! [ -z .. ].


In padd.sh line 643:
  printf "$@"
         ^--^ SC2059: Don't use variables in the printf format string. Use printf '..%s..' "$foo".


In padd.sh line 1038:
        echo $(sed 's/^"\|"$//g' <<< "${BASH_REMATCH[1]}")
             ^-- SC2046: Quote this to prevent word splitting.
             ^-- SC2005: Useless echo? Instead of 'echo $(cmd)', just use 'cmd'.
               ^-- SC2001: See if you can use ${variable//search/replace} instead.

For more information:
  https://www.shellcheck.net/wiki/SC2046 -- Quote this to prevent word splitt...
  https://www.shellcheck.net/wiki/SC2059 -- Don't use variables in the printf...
  https://www.shellcheck.net/wiki/SC2001 -- See if you can use ${variable//se...

SC2046 is a warning and not an info anymore…

… a good starting point might be https://github.com/marketplace/actions/shellcheck-action

This is pretty much the same (runs-on: ubuntu-latest) as this PR 🤔

Whatever we'd need to customize in the output format could then be done here.

I would only check and fail for severity=warning and error - not for info or even style - to improve code-quality.

[DL6ER]

This is pretty much the same (runs-on: ubuntu-latest) as this PR

Yes, but it does add the comments to the lines, hence, making it user-friendly. The extra resources seem justified if we can get everything.

---

I would only check and fail for severity=warning and error - not for info or even style - to improve code-quality.

For other parts of the code, e.g., then central daemon FTL, we fail hard for warnings/infos of any severity. This enforces code-quality ;-)

For the shellcheck manpage:

-S SEVERITY, --severity=severity
   Specify minimum severity of errors to consider. Valid values in order of severity are error, warning, info and style. The default is style.

And I wouldn't change this default. Otherwise, the output locally and the one from the CI may not be the same.

[thomasmerz]

Offtopic now? What's really cool with stickler: it runs only 2-4 seconds instead of up to 20 seconds with any shellcheck solution and it doesn't cost any GitHub Action Minutes 👍🏻 👍🏻
.
.
.

This is pretty much the same (runs-on: ubuntu-latest) as this PR

Yes, but it does add the comments to the lines, hence, making it user-friendly. The extra resources seem justified if we can get everything.

Currently I'm still a bit confused:

Please have a look at my test-run: thomasmerz#3

• Personally I really miss the SC-Codes a little bit (but you don't want or like them so much as I do) for better understanding of the warnings etc. Can these been shown, too?
• "All checks have passed", but shellcheck complained a lot.
• Direct link to annotations in the file changed" only works when clicking on the "right" "check details":
  First won't work,
  Second will work.
  If only one of them (the "right" one!?) won't have passed all checks, it should be ok. So this looks like a consequential error.

Can you please help and give my an advice?

For other parts of the code, e.g., then central daemon FTL, we fail hard for warnings/infos of any severity. This enforces code-quality ;-)

You are more in coding than me - I believe you that even severity=info makes code better.

And I wouldn't change this default. Otherwise, the output locally and the one from the CI may not be the same.

"Whose" local output? Yours or mine or from any other committer/contributor? 😉

[DL6ER]

We encourage users to run shellcheck locally before pushing. This will avoid complaints being mentioned in the first place.

The best solution seems to be: Install stickler for this repo, ultimately enforcing a certain quality of new contributions. Even when the output of stickler is less than what you get on the terminal you can use the latter to fix your issue (this is what I meant with "local") before pushing (again).

[thomasmerz]

Okay, please give me some time to rewrite my PR for stickler and checking my follow-up-PR #190 again.

[thomasmerz]

Here we go - good things take some time to reduce them to the max 😁 🎉

[yubiuser]

@DL6ER

If you agree with adding stickler here please merge and add the repo in the stickler-ci.com configuration (I can't do it.)