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 'Top Clients' name resolution from hosts file #336

Closed
wants to merge 1 commit into from
Closed

Add 'Top Clients' name resolution from hosts file #336

wants to merge 1 commit into from

Conversation

atesca09
Copy link

@atesca09 atesca09 commented Jan 5, 2017

By submitting this pull request, I confirm the following (please check boxes, eg [X] - no spaces) 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 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


Adds the client name resolution from the queries page to the index page 'Top Clients' list aswell

@CLAassistant
Copy link

CLAassistant commented Jan 5, 2017

CLA assistant check
All committers have signed the CLA.

@AzureMarker
Copy link
Contributor

Top Clients should already reverse lookup the hostnames from /etc/hosts if you check the setting in the settings page.

@AzureMarker
Copy link
Contributor

@atesca09 Does this PR do something new?

@atesca09
Copy link
Author

atesca09 commented Jan 8, 2017

I was looking for the resolve client names option and couldnt find it :(

With this PR the Top Clients are displayed in the same format as on the Queries Log page ( (<ip))
But I'd recommend closing this PR and changing the format in the requestIPs function

@DL6ER
Copy link
Member

DL6ER commented Jan 8, 2017

When you enable API_GET_CLIENT_HOSTNAME, than the function resolveIPs will exactly do that (resolve the IPs to hostnames). This resolution includes all data that the Pi-hole knows from its /etc/hosts file.

This data is formatted like hostname@IP and parsed by the JS script to show only hostname. Do I understand this correctly, that you would rather like to see hostname(IP)?. This will be a very long string and probably don't fit well into the tables.

Closing this PR as suggested by @atesca09

@DL6ER DL6ER closed this Jan 8, 2017
@DL6ER
Copy link
Member

DL6ER commented Jan 8, 2017

@atesca09 Please have a look at #346

@atesca09
Copy link
Author

It helps a bit, but I still prefer the hostname(IP) format like it is on the query log pages

@DL6ER
Copy link
Member

DL6ER commented Jan 14, 2017

We decided to get rid of that because of the table looking really bad for people using IPv6 in their internal network. It is no problem for a line like:

somehost.local(192.168.1.10)

However,

android-db4fb31512299e9971cf.local(fe80:0000:0000:0000:0000:0000:c0a8:010a)

is certainly a problem.

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.

4 participants