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

Major overhaul of install script output. #1471

Merged
merged 74 commits into from Jun 21, 2017

Conversation

Projects
None yet
6 participants
@PromoFaux
Copy link
Member

PromoFaux commented May 14, 2017

By submitting this pull request, I confirm the following (please check boxes, eg [X])Failure to fill the template will close your PR:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

  • 10 (very familiar)

install

(the gif went all spangly!)

This template was created based on the work of udemy-dl.

PromoFaux added some commits May 14, 2017

RED

@PromoFaux PromoFaux requested review from dschaper , WaLLy3K and DL6ER May 14, 2017

@PromoFaux

This comment has been minimized.

Copy link
Member Author

PromoFaux commented May 15, 2017

image

PromoFaux added some commits May 15, 2017

@PromoFaux

This comment has been minimized.

Copy link
Member Author

PromoFaux commented May 15, 2017

pihole disable

@PromoFaux

This comment has been minimized.

Copy link
Member Author

PromoFaux commented May 15, 2017

pihole logging

Do not output colours for non-terminal instances
* `-t 1` is true if a Terminal instance is calling the script, `-t` does not return `1` from say, being called from cron
* `$(tput colors)` confirms the Terminal is capable of showing colours

@PromoFaux PromoFaux force-pushed the tweak/colourAllTheThings branch from cf771a4 to 39d27bc May 16, 2017

PromoFaux added some commits May 17, 2017

error checking on $tt. Make sure it is a number. Also only accept arg…
…uments that end with an s or an m, not just contain one.
COL_YELLOW='\e[1;33m'
COL_GRAY='\e[0;30m'
COL_LIGHT_GRAY='\e[0;37m'
else

This comment has been minimized.

@PromoFaux

PromoFaux May 17, 2017

Author Member

I'm wondering about this, I think we can just get away with the first part of the if block, don't think we need the else part!

This comment has been minimized.

@WaLLy3K

WaLLy3K May 18, 2017

Collaborator

If we're just echo -e'ing these variables, then it's safe to not have the else block as echo won't spit an error message if the variable isn't found.

PromoFaux added some commits May 17, 2017

COL_YELLOW='\e[1;33m'
COL_GRAY='\e[0;30m'
COL_LIGHT_GRAY='\e[0;37m'
else

This comment has been minimized.

@WaLLy3K

WaLLy3K May 18, 2017

Collaborator

If we're just echo -e'ing these variables, then it's safe to not have the else block as echo won't spit an error message if the variable isn't found.

@@ -49,14 +53,14 @@ GitCheckUpdateAvail() {
REMOTE="$(git rev-parse @{upstream})"

if [[ ${#LOCAL} == 0 ]]; then
echo "::: Error: Local revision could not be obtained, ask Pi-hole support."
echo "::: Additional debugging output:"
echo -e " ${COL_LIGHT_RED}Error: Local revision could not be obtained, ask Pi-hole support."

This comment has been minimized.

@WaLLy3K

WaLLy3K May 18, 2017

Collaborator

Just double checking with you, but is the 8 space indenting on error lines like 56/57/62/63/101/102/133/134/180/194 intentional?

This comment has been minimized.

@PromoFaux

PromoFaux May 18, 2017

Author Member

Yes, but for no real reason other than to make it stand out as an error message

updatesToInstall=$(eval "${PKG_COUNT}")
echo " done!"
echo ":::"
#echo -e "\r\033[K ${TICK} ${str}"

This comment has been minimized.

@WaLLy3K

WaLLy3K May 18, 2017

Collaborator

Is this intentionally commented out?

WaLLy3K added some commits May 19, 2017

Display "list type" as name, not filename
* To make output fit on standard Terminal screen

@dschaper dschaper added the Hold label May 24, 2017

@DL6ER DL6ER added the Merge Conflict label Jun 3, 2017

@WaLLy3K
Copy link
Collaborator

WaLLy3K left a comment

pihole disable 5z is valid

pihole -w <domain> returns errors:
.rm: cannot remove ‘/etc/pihole/local.list’: No such file or directory
/opt/pihole/gravity.sh: line 363: gravity_doHostFormat: command not found
/opt/pihole/gravity.sh: line 370: gravity_doHostFormat: command not found
/opt/pihole/gravity.sh: line 380: gravity_doHostFormat: command not found
mv: cannot stat ‘/etc/pihole/black.list.tmp’: No such file or directory
rm: cannot remove ‘/etc/pihole/pihole.*.txt’: No such file or directory

pihole -a -c/f/k does not return output

pihole -i output is not consistent with style

pihole -l error output is not consistent with style

pihole -up output issue:

  [i] Performing reconfiguration, skipping download of local repos
  [i] Resetting repository within /etc/.pihole...\r\033[K  [✓] Resetting repository within /etc/.pihole...  [i] Resetting repository within /var/www/html/admin...\r\033[K  [✓] Resetting repository within /var/www/html/admin...  [i] Main Dependency checks...
  [✓] Checking for bc

WaLLy3K added some commits Jun 21, 2017

Colour Temp/Interface output
Also shortened interface strings to fit on 80 char Terminal screen
Made `pihole disable 5z` invalid & other fixes
* Removed two excess spaces
* Quoted and braced $tt variable
* Fixed newline output for "Disabling/Enabling blocking"
* Added error fallback if invalid argument (not s/m) is detected
* Made disable error consistent
* Made logging error consistent
* Quoted "$2" for consistency
Re-merged script with development
* Updated help text
* Modified some comments
* Cleaned up extra spaces and indenting
* Quoted and braced variables where appropriate
* L185/286: Replaced echo with redirect
* User agents for adblock.mahakala.is/adaway.org unnecessary 
* Updated text to use colour output
Standardised 'pihole enable/disable' output
* Also fixed broken 'pihole disable' behaviour
if ! fully_fetch_repo "${PI_HOLE_FILES_DIR}" ; then
echo "::: Fetching all branches for Pi-hole core repo failed!"
echo -e " ${CROSS} $str"
exit 1
fi
corebranches=($(get_available_branches "${PI_HOLE_FILES_DIR}"))

This comment has been minimized.

@WaLLy3K

WaLLy3K Jun 21, 2017

Collaborator

If this function fails, it will return the following to the user (and I'm not sure how to work around this, off the top of my head)

[i] Fetching branches from https://github.com/pi-hole/pi-hole.gitfatal: unable to access 'https://github.com/pi-hole/pi-hole.git/': Could not resolve host: github.com
Add default options & fix pihole user logic
* Add default option when removing Pi-hole
* Quoted variables
* Fixed pihole user logic
* Add default option when removing dependencies
@WaLLy3K
Copy link
Collaborator

WaLLy3K left a comment

pihole -h

Whitelist/Blacklist Options:
pihole -w foo.bar.baz
pihole -w -d foo.bar.baz
pihole -b foo.bar.baz
pihole -b -d foo.bar.baz
pihole -wild bar.baz
pihole -wild -d bar.baz

Debugging Options:
pihole -f
pihole -r
pihole -t

Admin Options:
pihole -a -p foo
pihole -a -p
pihole -a -c/k/f
pihole -a -i

Options:
pihole -g
pihole -g -f
pihole -l
pihole -l on
pihole -l off
pihole -q
pihole -q doubleclick.net
pihole -q doubleclick.net -exact
pihole status
pihole enable
pihole disable
pihole disable 5s

pihole restartdns
pihole checkout

pihole uninstall

This checkout issue is the last issue that I'm able to find.

Check for existence of COL_TABLE
COL_TABLE will not have been copied to /opt/pihole if webpage.sh is called from initial basic-install.sh run
Print 'git remote show origin' STDERR on new line
* Put source on new line
* Store 'git remote show origin' STDERR as STDOUT variable
* Minor consistency tweaks
* Replaced "AdminLTE" wording with "Web Admin"
* Print STDERR output from get_available_branches
@WaLLy3K
Copy link
Collaborator

WaLLy3K left a comment

No more known issues

@WaLLy3K WaLLy3K merged commit 536585b into development Jun 21, 2017

5 of 6 checks passed

ci/circleci No test commands were found
Details
codacy/pr Good work! A positive pull request.
Details
code-review/pullapprove Approved by PromoFaux, WaLLy3K
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@WaLLy3K WaLLy3K deleted the tweak/colourAllTheThings branch Jun 21, 2017

@Th3M3

This comment has been minimized.

Copy link

Th3M3 commented Jun 21, 2017

Looks very good.
But I think I have found a wrong styled line.
Please have a look to lineRemoving style/vendor/old.skin-blue.min.css:

script

@WaLLy3K WaLLy3K referenced this pull request Jun 22, 2017

Closed

Issues with Colour Output (Wording/Layout/Newlines/Etc) #1548

3 of 3 tasks complete
@WaLLy3K

This comment has been minimized.

Copy link
Collaborator

WaLLy3K commented Jun 22, 2017

Thanks @Th3M3, I've opened #1548 as a place we can look into the issues further! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.