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] Store all gravity whitelist and blacklist vars in pihole #826

Closed
wants to merge 14 commits into from
Closed

[WIP] Store all gravity whitelist and blacklist vars in pihole #826

wants to merge 14 commits into from

Conversation

tuplink
Copy link
Contributor

@tuplink tuplink commented Oct 20, 2016

By submitting this pull request, I confirm the following (please check boxes, eg [X]):

  • 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?:

  • 1 (very unfamiliar)
  • 2
  • 3
  • 4
  • 5
  • 6
  • 7
  • 8
  • 9
  • 10 (very familiar)

Store all script vars in pihole

@@ -53,8 +50,8 @@ HandleOther(){

PopBlacklistFile(){
#check blacklist file exists, and if not, create it
if [[ ! -f ${blacklist} ]];then
touch ${blacklist}
if [[ ! -f ${blacklistFile} ]];then
Copy link
Member

Choose a reason for hiding this comment

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

These need to be in the format of "${blacklistFile}" so that bash doesn't split on space if the filename contains a space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that mistake is all over

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you're cleaning up a lot of our messes that we needed to get to, so thank you thank you for that!!

#Source the setupVars from install script for the IP
export setupVars=/etc/pihole/setupVars.conf

export basename=pihole
Copy link
Member

Choose a reason for hiding this comment

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

I believe exporting all of these variables and values will leave them in the environment even after the script is finished executing. I'll need to check on that however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pihole -w zzz.clickbank.net > /dev/null ; echo "basename=$basename"

basename=

Copy link
Member

Choose a reason for hiding this comment

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

What's the env look like?

@tuplink
Copy link
Contributor Author

tuplink commented Oct 20, 2016

probably better to just ignore this one till a few are taken care of first cause i imagin its going to get lots of conflicts

@dschaper
Copy link
Member

There's two ways you can put a hold on a merge, you can tag/label WIP for Work In Progress, or I believe we have it set so that the PR author needs to approve the merge before PullApprove will release it...

@tuplink
Copy link
Contributor Author

tuplink commented Oct 20, 2016

or i just remove the branch?

@dschaper
Copy link
Member

So far there are no conflicts, so I wouldn't close out the PR...

@tuplink tuplink closed this Oct 20, 2016
@tuplink
Copy link
Contributor Author

tuplink commented Oct 20, 2016

gonna any ways alot of quotes need fixed 👍

@tuplink tuplink reopened this Oct 20, 2016
@tuplink tuplink closed this Oct 20, 2016
@tuplink tuplink deleted the RemoveVARSinGrav branch October 20, 2016 18:19
@tuplink tuplink restored the RemoveVARSinGrav branch October 20, 2016 18:20
@tuplink tuplink changed the title Store all gravity whitelist and blacklist vars in pihole [WIP] Store all gravity whitelist and blacklist vars in pihole Oct 20, 2016
@tuplink
Copy link
Contributor Author

tuplink commented Oct 20, 2016

export command is used to export a variable or function to the environment of all the child processes running in the current shell.

@tuplink tuplink reopened this Oct 20, 2016
@tuplink
Copy link
Contributor Author

tuplink commented Oct 20, 2016

So only processes spawned from where its called

@dschaper
Copy link
Member

Okay cool, good to know.

@dschaper dschaper added the PR: Merge Conflict Issue blocking check and merge of code label Nov 5, 2016
@AzureMarker
Copy link
Contributor

@tuplink @pi-hole/gravity Is this now obsolete with #859?

@tuplink
Copy link
Contributor Author

tuplink commented Nov 9, 2016

@Mcat12 mostly

basename=pihole
piholeDir=/etc/${basename}
whitelist=${piholeDir}/whitelist.txt
blacklist=${piholeDir}/blacklist.txt

would be moved to setupVars and setupVars would be sourced from all scripts

and basic-install would need to reflect the changes to setupvars

@AzureMarker
Copy link
Contributor

And the web interface php files would need to take their paths from setupVars

@tuplink
Copy link
Contributor Author

tuplink commented Nov 9, 2016

Haven't looked into that. Should probably build that into pihole script For the list. And use the pihole command to edit

@AzureMarker
Copy link
Contributor

The web interface already uses the pihole command to edit the lists, but other files are hardcoded, for example gravity.list and the whitelist/blacklist files.

@tuplink
Copy link
Contributor Author

tuplink commented Nov 9, 2016

so,
TODO part 1:
blacklist.sh
FIX output of pihole -b -l (outputs lots of extra stuff)
whitelist.sh
FIX output of pihole -w -l (outputs lots of extra stuff)
ADMIN-LTE
make use of "pihole -b -l" and "pihole -w -l"

TODO part 2
move gravity whitelist and blacklist vars to setupVars

@AzureMarker
Copy link
Contributor

I would start a whole new branch using the latest code, which has the white and black lists in list.sh

@tuplink
Copy link
Contributor Author

tuplink commented Nov 9, 2016

will start on this monday

@AzureMarker AzureMarker closed this Apr 6, 2017
@dschaper dschaper removed PR: Merge Conflict Issue blocking check and merge of code WIP Work in progress. labels May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants