Fix Issue 36 🚨 shellcheck'ed

[thomasmerz]

This fixes and closes issue #36.
I checked/validated by running pihole_adlist_tool -d 1 -t 10 -u -a with old and new version and diff'ed the results:

$ sdiff -s old new
							      >	  [i]  There is a mismatch between your enabled adlists and t
							      >	       You have 30 adlists enabled, but data from 0
                                                                                                                   							      >	       You're likely disabled/enabled adlist without running
							      >	       It's highly recommended to run gravity now to solve th
							      >
							      >
							      >	  Would you like to run gravity now?
							      >
							      >	  1)  Yes
							      >	  2)  No
							      >
							      >	  [i]  Running in automatic mode, not running gravity.
							      >
							      >
							      >
  [i]  Since 19.01.2022 18:59:55 529 different domains |	  [i]  Since 19.01.2022	19:00:09 529 different domains
  [i]  Using you current adlist configuration 551 domains|	  [i]  Using you current adlist configuration 551 domains
                                                                                                                                Those would have been the 10 top blocked adlist dom |	       Those would have been the 10 top blocked adlist dom
gs-loc.ls-apple.com.akadns.net       618 		      |	gs-loc.ls-apple.com.akadns.net       620
self.events.data.microsoft.com       353 		      |	self.events.data.microsoft.com       354
1   1        97346          273              6156          8  |	1   1        97346          273              6160          8
36  1        1095098        301              5678          35 |	36  1        1095098        301              5682          35
43  1        1026181        98               2864          6  |	43  1        1026181        98               2868          6
61  1        234            61               3534          51 |	61  1        234            61               3538          51
63  1        404455         355              10451         55 |	63  1        404455         355              10456         55
  [✓]  Temporary database removed			      |	  [i]  Temporary database remove

For privacy reasons I can't get you full output. Maybe you can check this with your Pi-Hole… 🤔

[yubiuser]

Thanks for checking. I need to to look at this properly, but improvements are always welcome

[thomasmerz]

@yubiuser , is it now ok for you and can this be merged and issue #36 be closed? :)

[yubiuser]

Please be patient. I need some time to test it locally. If it works, I'll merge it. In principal, there is nothing that stands against it - it's just a matter of time.

[yubiuser]

There are still some un-resolved suggestions - did you miss those or was there a reason you did not include them?
I'm not very familiar with shell check, what is to reason to include

# shellcheck disable=SC2068
# shellcheck disable=SC2145
# shellcheck disable=SC2027
# shellcheck disable=SC2046
# shellcheck disable=SC2125
# shellcheck disable=SC2128
# shellcheck disable=SC2166
# shellcheck disable=SC2207

at the beginning instead of fixing the issues?

[thomasmerz]

There are still some un-resolved suggestions - did you miss those or was there a reason you did not include them?

To be honest: I'm lacking time and some of these checks are more difficult to fix 🤷🏻‍♂️
But you are right: you deserve a completely shellcheck'ed version - give me some time and you will get another commit in this PR…

[yubiuser]

No need to rush. It's done when it's done. I've installed shellcheck locally and hope to avoid such a mess in the future

[thomasmerz]

A local installation/container might be sufficient, but I sometimes forget in a hurry to check locally and I'm thankful for having a GitLab CI pipeline job for my (private) GitLab repos with this .gitlab-ci.yml (this might be slightly different on GitHub Actions with a .github/workflows/shellcheck.yml 🤷🏻‍♂️):

---
stages:
  - shellcheck

# ---
# https://github.com/koalaman/shellcheck
# ---
# Excluded checks:
# https://www.shellcheck.net/wiki/SC1090 -- Can't follow non-constant source....
# https://www.shellcheck.net/wiki/SC1091 -- Not following: /etc/rc.status was...
# ---
shellcheck:
  allow_failure: false
  stage: shellcheck
  image: "docker.io/koalaman/shellcheck-alpine"
  script:
    - |
      set +e
      apk add file
      find . -type f | grep -vE \
        "some-scripts-that-I-know-why-not-being-checked" \
        | while read -r sh; do
        if [ "$(file --brief --mime-type "$sh")" == 'text/x-shellscript' ]; then
          echo "Checking $sh"
          if ! shellcheck --color=always --severity=warning --exclude=SC1090,SC1091 "$sh"; then
            touch some_scripts_have_failed_shellcheck
          fi
        else
          true
        fi
      done
      if [ -f ./some_scripts_have_failed_shellcheck ]; then
        echo "WARN: Shellcheck failed for one or more shellscript(s) :-("
        exit 1
      else
         echo "INFO: All your shellscripts are OK :-)"
      fi
# ---

If you don't know how to do this - I might also make a PR for you if I find out how this works on GitHub (just because I also want to know HOW to do this) 😁

[thomasmerz]

I couldn't go to bed before I didn't know how GH action can handle this 😉
So, please have a look at my PR #38 and merge at will 👍🏻

[thomasmerz]

@yubiuser , please review my additions. Thanks :)

[yubiuser]

Please do not always force-push. This conceals what has changed in each commit. Please just do a normal push

[thomasmerz]

Please do not always force-push. This conceals what has changed in each commit. Please just do a normal push

Ok, I saw your recommendation after my last force-push. I pledge to do better 🙏🏻

[yubiuser]

I created a new branchdevelopment and set it as the new base branch. All PRs should be made against this now. So I can "collect" several PRs and release at once. I already changed the target of this PR.

I hope to finish review for this today.

[thomasmerz]

Sounds great (dev-branch and the rest, too) 👍🏻

[yubiuser]

Except of these two comments, the code looks fine now.
However, I've not tried it yet. Did you run the script - does it break something?

[thomasmerz]

However, I've not tried it yet. Did you run the script - does it break something?

Yes, I did test it with all commits. Here are my final test results with -d 2 -t 10 -a:

Running outside docker container:

sdiff -s shows no significant diffs on my "productive" Pi-hole:

$ sdiff -s old.txt new.txt
  *** Pihole Adlist Tool 2.6 ***			      |	  *** Pihole Adlist Tool 2.6.1 ***
  [i]  Since 17.02.2022 14:44:52 869 different domains |	  [i]  Since 17.02.2022	14:37:42 869 different domains
  [i]  Using you current adlist configuration 880 domains|	  [i]  Using you current adlist configuration 880 domains
                                                                                                                                Those would have been the 10 top blocked adlist dom |	       Those would have been the 10 top blocked adlist dom
app-measurement.com                 3126		      |	app-measurement.com                 3098
gsp64-ssl.ls-apple.com.akadns.net   2325		      |	gsp64-ssl.ls-apple.com.akadns.net   2326
gs-loc.ls-apple.com.akadns.net      2227		      |	gs-loc.ls-apple.com.akadns.net      2228
self.events.data.microsoft.com      2015		      |	self.events.data.microsoft.com      2016
gs-loc-new.ls-apple.com.akadns.net  1098		      |	gs-loc-new.ls-apple.com.akadns.net  1100
firebaselogging-pa.googleapis.com   1000		      |	firebaselogging-pa.googleapis.com   998
frontier-va.tiktokv.com             672 		      |	frontier-va.tiktokv.com             658
mon.tiktokv.com                     624 		      |	mon.tiktokv.com                     610
1   1        102022         439              15842         14 |	1   1        102022         439              15816         14
11  1        10346          91               6217             |	11  1        10346          91               6199
30  1        767            61               9977          6  |	30  1        767            61               9921          6
36  1        1160731        545              18257         73 |	36  1        1160731        545              18227         73
42  1        353141         151              6034          9  |	42  1        353141         151              6006          9
43  1        1090045        144              8187          7  |	43  1        1090045        144              8153          7
63  1        406653         596              24306         10 |	63  1        406653         596              24252         10
64  1        95693          24               665           11 |	64  1        95693          24               671           11
66  1        184            8                4685          4  |	66  1        184            8                4627          4

Running inside docker container:

sdiff -s shows diffs on my "productive" Pi-hole regarding "mismatch between your enabled adlists" 🤷🏻‍♂️ I'm not sure right now how this comes…

$ sdiff -s old_docker.txt new_docker.txt
  [i]  There is a mismatch between your enabled adlists       <
       You have 30 adlists enabled, but data from 0							      <
       You're likely disabled/enabled adlist without run      <
       It's highly recommended to run gravity now to sol      <
							      <
							      <
  Would you like to run gravity now?    		      <
							      <
  1)  Yes       					      <
  2)  No						      <
							      <
  [i]  Running in automatic mode, not running gravity.        <
							      <
							      <
							      <
  [i]  Since 17.02.2022	14:44:41 869 different domains        |	  [i]  Since 17.02.2022	14:49:43 869 different domains
  [i]  Using you current adlist configuration 880 domains							      |	  [i]  Using you current adlist configuration 880 domains
       Those would have been the 10 top blocked adlist d      |	       Those would have been the 10 top blocked adlist dom
app-measurement.com                 3128		      |	app-measurement.com                 3146
gsp64-ssl.ls-apple.com.akadns.net   2325		      |	gsp64-ssl.ls-apple.com.akadns.net   2329
gs-loc.ls-apple.com.akadns.net      2227		      |	gs-loc.ls-apple.com.akadns.net      2231
self.events.data.microsoft.com      2015		      |	self.events.data.microsoft.com      2019
gs-loc-new.ls-apple.com.akadns.net  1098		      |	gs-loc-new.ls-apple.com.akadns.net  1102
firebaselogging-pa.googleapis.com   1000		      |	firebaselogging-pa.googleapis.com   998
frontier-va.tiktokv.com             672 		      |	frontier-va.tiktokv.com             682
mon.tiktokv.com                     624 		      |	mon.tiktokv.com                     634
gs-loc.apple.com                    444 		      |	pancake.cdn-apple.com.akadns.net    445
1   1        102022         439              15844            |	1   1        102022         439              15863         14
11  1        10346          91               6219             |	11  1        10346          91               6237
30  1        767            61               9981             |	30  1        767            61               10015         6
36  1        1160731        545              18259            |	36  1        1160731        545              18275         73
42  1        353141         151              6036             |	42  1        353141         151              6052          9
43  1        1090045        144              8189             |	43  1        1090045        144              8205          7
61  1        234            63               12000            |	61  1        234            63               12005         53
63  1        406653         596              24308            |	63  1        406653         596              24332         10
66  1        184            8                4687             |	66  1        184            8                4727          4
  [i]  Temporary database removed       		      |	  [✓]  Temporary database removed

[yubiuser]

The strange thing is: there is also text missing... it should continue with "(patially different) adlists in your gravity database."

See my comment below, why it is failing.

[thomasmerz]

BTW: if we will come to a happy end, do you like to have a "clean" git history with only one rebased commit to commit fa8e5ba ?

[yubiuser]

I think I will do a squash merge :)

[thomasmerz]

Current diff:

running pihole_adlist_tool inside docker container:

$ sdiff -s old_docker.txt new_docker.txt 
  *** Pihole Adlist Tool 2.6 ***			      |	  *** Pihole Adlist Tool 2.6.1 ***
  [i]  Since 18.02.2022 11:52:59 893 different domains        |	  [i]  Since 18.02.2022	11:52:59 893 different domains
       Those would have been the 10 top blocked adlist d      |	       Those would have been the 10 top blocked adlist dom

running pihole_adlist_tool outside docker container:

$ sdiff -s old.txt new.txt 
  *** Pihole Adlist Tool 2.6 ***			      |	  *** Pihole Adlist Tool 2.6.1 ***
  [i]  Since 18.02.2022 11:54:39 893 different domains |	  [i]  Since 18.02.2022	11:54:25 893 different domains
  [i]  Using you current adlist configuration 890 domains	  [i]  Using you current adlist configuration 890 domains
hose would have been the 10 top blocked adlist dom |	       Those would have been the 10 top blocked adlist dom
1   1        102022         438              16962         12 |	1   1        102022         438              16974         12
30  1        767            60               10843         6  |	30  1        767            60               10847         6 
36  1        1091461        562              20232         89 |	36  1        1091461        562              20276         89
42  1        353140         151              6719          11 |	42  1        353140         151              6723          11
60  1        378            20               2087          10 |	60  1        378            20               2095          10
61  1        234            64               11202         54 |	61  1        234            64               11206         54
63  1        406494         596              26853         10 |	63  1        406494         596              26881         10

[yubiuser]

So no real differences ;-)