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

Cleanup uninstall script #1663

Merged
merged 11 commits into from Sep 21, 2017

Conversation

Projects
None yet
3 participants
@PromoFaux
Copy link
Member

PromoFaux commented Aug 17, 2017

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

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have written tests and verified that they fail without my change.
  • I have squashed any insignificant commits.
  • This change has comments for package types, values, functions, and non-obvious lines of code.
  • 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 Signed Off all commits. (git commit --signoff)

Please explain what you have done and wish to accomplish with this Pull Request

  1. What does this change do, exactly?
    De-duplicates dependency list (we're always forgetting to update the dep list in uninstall.sh when we add one to basic-install.sh)

This change sources the install script and uses the distro_check function to generate the same list as the the install script.

Also added to the output a list of each package that may have been installed by the installer, allowing the user to quickly decide if they wish to go through it and uninstall one or many (or all), or just leave them all installed.

dependencyloop

noloop

  1. Please link to the relevant issues.

  2. Which documentation changes (if any) need to be made because of this PR?

PromoFaux added some commits Aug 17, 2017

Initial overhaul of uninstall script sourcing `distro_check()` from `…
…basic-install.sh` Totally untested as yet...

Signed-off-by: Adam Warner <adamw@rner.email>
No need to declare `PKG_MANAGER`, as it's already declared in basic-i…
…nstall

Signed-off-by: Adam Warner <adamw@rner.email>
need to include `PH_TEST="true"` so that the install script isn't run…
… when sourcing it.. doh

Signed-off-by: Adam Warner <adamw@rner.email>
Add "${INSTALLER_DEPS[@]}" to package array
add `-f` to rm commands so that `set -e` is not kicked

Signed-off-by: Adam Warner <adamw@rner.email>
verbiage
Signed-off-by: Adam Warner <adamw@rner.email>

@PromoFaux PromoFaux requested a review from Aug 17, 2017

accidentally a space
Signed-off-by: Adam Warner <adamw@rner.email>
for i in "${DEPS[@]}"; do
echo -n "${i} "
done
echo "${COL_NC}"git pu

This comment has been minimized.

@WaLLy3K

WaLLy3K Aug 23, 2017

Collaborator

git pu

Is this a typo?

This comment has been minimized.

@PromoFaux

PromoFaux Aug 23, 2017

Author Member

yeah... How the hell did that get there?!

@PromoFaux PromoFaux requested a review from WaLLy3K Aug 24, 2017

@PromoFaux PromoFaux assigned WaLLy3K and unassigned WaLLy3K Aug 24, 2017

@PromoFaux PromoFaux requested a review from Sep 9, 2017

Merge branch 'development' into new/UninstallCleanup
Signed-off-by: Adam Warner <adamw@rner.email>

# Conflicts:
#	automated install/uninstall.sh

@PromoFaux PromoFaux added this to the v3.2 milestone Sep 9, 2017

@@ -198,7 +208,13 @@ else
echo -e " ${INFO} Be sure to confirm if any dependencies should not be removed"
fi
while true; do
read -rp " ${QST} Do you wish to go through each dependency for removal? [Y/n] " yn
echo -e "${COL_YELLOW} ${INFO} The following dependencies may have been added to the system by Pi-hole install:"

This comment has been minimized.

@WaLLy3K

WaLLy3K Sep 20, 2017

Collaborator

${COL_YELLOW} should be placed after ${INFO} and the line should be limited to 80 characters.

I propose: echo -e " ${INFO} ${COL_YELLOW}The following dependencies may have been added by the Pi-hole install:"

This comment has been minimized.

@PromoFaux

PromoFaux Sep 20, 2017

Author Member

feel free to push that change on my behalf, please!

Tweak wording to fit on 80 character screen
* Also move colour after $INFO
@WaLLy3K
Copy link
Collaborator

WaLLy3K left a comment

Sourcing the script in this manner worked nicely in Debian Stretch. Haven't tested other distros - would recommend trying CentOS before merging (if it hasn't already!)

@PromoFaux PromoFaux merged commit a643f40 into development Sep 21, 2017

5 checks passed

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

@PromoFaux PromoFaux deleted the new/UninstallCleanup branch Sep 21, 2017

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.