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

add pihole module. #874

Merged
merged 1 commit into from
Apr 25, 2020
Merged

Conversation

jonhadfield
Copy link
Contributor

Hi, this is a new module to display information about a running a Pi-hole server.
This doesn't introduce any new package dependencies but calls the Pi-Hole API endpoint using the standard library and displays them using the already included tablewriter library.
As a first attempt at writing a module, I've based this on existing modules, so not sure I'm following best practices for this project.
Submitting PR for docs shortly.
Any feedback appreciated.

@senorprogrammer
Copy link
Collaborator

If you run go mod tidy on your PR, the CI checks should pass.

@senorprogrammer
Copy link
Collaborator

I want to use this, but I haven't been able to get past the following error:

failed to connect to Pi-hole server: Get "?version=": unsupported protocol scheme ""

with an API URL of: http://192.168.x.xx/admin/api.php

Any thoughts?

I also recommend having your error messages output the error itself, instead of url.String(). I replaced the failing one with

return fmt.Errorf(" failed to connect to Pi-hole server: %v", err.Error())

to get the message above.

@jonhadfield
Copy link
Contributor Author

jonhadfield commented Apr 19, 2020

Edit: I've just realised the documentation says to use apiURL whereas the code is looking for apiUrl. I'll update code and docs accordingly.

To start with the error itself, in your web client you should be able to do a GET with http://192.168.x.x/admin/api.php?version (no auth token required) to get a json response with the version number.
According to the API doc this should work with both API versions 2 and 3 (TBH, I've not tried against version 2 and just trusted the doc). The spurious = after ?version is an artefact of using url.Values to build query strings, so I might just drop that for this call. Please could you also check you're not missing a port number and are really running on port 80?

I tried to avoid outputting the full error due to the amount of space available and, probably naively, trusting there weren't enough scenarios where it would be useful. I'm also concious of the full error disclosing the API URL and, most importantly, token if used outside of a home environment. What I'll do is output the error but strip the token from any error responses.
I'll also include a message about a potentially missing port number though as it might catch some users out.

@jonhadfield
Copy link
Contributor Author

I've made the changes mentioned above and all errors are now output but auth token is stripped.
wtf docs PR updated: wtfutil/wtfdocs#87
Rebased against master.

@senorprogrammer
Copy link
Collaborator

That is working nicely!

@senorprogrammer senorprogrammer merged commit daf1e61 into wtfutil:master Apr 25, 2020
@senorprogrammer
Copy link
Collaborator

Merged. This is an awesome addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants