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

added dns options for pi-hole rockon #178

Merged
merged 9 commits into from
Mar 11, 2019
Merged

Conversation

magicalyak
Copy link
Contributor

Updated from #156
Issue - #176

This is a continuation of the previous PR and adds the dns needed by @haioken
This still needs a test which I hope to do shortly but could use feedback on if others want to test.
I may also clean up the rockon a bit (DNS1 and 2 may be unnecessary and I need to verify TZ).

@magicalyak
Copy link
Contributor Author

added https://github.com/haioken request dnsmasq vol that @FroggyFlox mentioned
changed to official image
cleaned up options
specified no to IPv6 (you needed a lot more options to make it true anyway).

@phillxnet
Copy link
Member

phillxnet commented Feb 10, 2019

@magicalyak This looks good, thanks for getting this one moving again. Quite looking forward to merging this.

I've go one minor quick request though. We are trying to move towards the website element linking to the upstream project, ie in this case Pi-hole itself ie:
https://pi-hole.net/

and for the time being, at least until we do this programmatically or via an additional docker_hub_url json element or the like, we are embedding in the description via html a link to the docker hub image.

Please see @FroggyFlox recent Teamspeak3 json in repo for an example of this. It also 'fakes' via paragraph formatting a 'subtitle' containing this link. That way less technical users can link to site from the Title (website json element) and we also present the info available via the (docker hub) link within the description.

That would be dandy. Just trying to move towards all new submissions containing both links:
See issue #31 on this.

@magicalyak
Copy link
Contributor Author

Description changed

@magicalyak
Copy link
Contributor Author

lol had a typo - I am testing this now.

@magicalyak
Copy link
Contributor Author

magicalyak commented Feb 10, 2019

After fixing typo and index - Tests PASSED for me
ping @phillxnet

@phillxnet
Copy link
Member

@magicalyak Thanks for the description change. That's much better.

pi-hole-rock-on

But the title still links to the docker hub page. Could we possibly have this updated to the Pi-Hole's own project. That way we surface their main page complete with their donate options and the like. I.e. this looks good to me bar the "website": update to https://pi-hole.net.

Bar that, given @haioken and @FroggyFlox have had a hand in this pr's direction (see @laxdog's original work on the image change in the now deprecated pr #156), I'd like a quick review from either of them on the more functional side. I haven't used Pi-Hole before but these pull requests have peaked my interest.

@phillxnet phillxnet added the needs review Test install, function, on / off behaviour, all links / info. label Feb 11, 2019
@phillxnet
Copy link
Member

@magicalyak Thanks for the website element fix, much nicer.
Both links now work as intended.

We just need a quick review from @haioken or @FroggyFlox to make sure they are happy and I'll pop this in.

Well done persevering with this one. Will be great to have it done and dusted.

Copy link
Member

@FroggyFlox FroggyFlox left a comment

Choose a reason for hiding this comment

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

Thanks a lot @magicalyak for taking on this edits!

Thanks also for simplifying the json, I agree with you that there were some redundancies.

I'm OK with all changes, but besides the little cosmetic changes I suggested in the Github review, I also think we may even be able to get rid of DNS1 and DNS2 altogether as the user can still set these on the webUI directly (with simple checkboxes. Google's DNS (8.8.8.8 and 8.8.4.4) are used by default but one can unset these and/or add any they want from the webUI later.
Any thought?

pi-hole.json Outdated Show resolved Hide resolved
pi-hole.json Outdated Show resolved Hide resolved
FroggyFlox and others added 2 commits March 11, 2019 08:14
Co-Authored-By: magicalyak <[email protected]>
Co-Authored-By: magicalyak <[email protected]>
@magicalyak
Copy link
Contributor Author

@FroggyFlox that works for me, did you want to submit the commit or should we go as is?

@magicalyak
Copy link
Contributor Author

Removed the options, this should be good to go @phillxnet

@phillxnet
Copy link
Member

@magicalyak Thanks for rounding this one out, hopefully finally.
@FroggyFlox Your final thoughts on this one if you would. Are you happy with me merging as is, as per you last review here?

Thanks people, itching to get this one in so lets pull it to a close.

@FroggyFlox
Copy link
Member

Thanks again @magicalyak for your orignal PR and all the edits!

@phillxnet, the commits since my Github review look good to me, and after a quick test on Rockstor-3.9.1-16, it installs OK, starts OK, and the webUI is accessible without error through Rockstor's webUI button.

@phillxnet phillxnet removed the needs review Test install, function, on / off behaviour, all links / info. label Mar 11, 2019
@phillxnet
Copy link
Member

Thanks to @magicalyak for pulling this one together and to @laxdog (original pr #156) @FroggyFlox for multiple reviews / suggestions, @haioken for advise / input.

Fixes #176

@phillxnet phillxnet merged commit 02d7834 into rockstor:master Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants