Request: add comment field to client table

General Beta 5.0
1

I'm trying to get acquainted with group management.

When entering clients by IP address, it rapidly becomes unclear which client represents a device, the table looks like this:

image
image1947×1022 19.4 KB

Would it be possible to add an optional comment field.

I'm using a script to create clients, source, something like:

192.168.2.57	raspberry57.localdomain		raspberry57
192.168.2.99	tx-l42d25e.localdomain		tx-l42d25e
192.168.2.100	ps3.localdomain			ps3
192.168.2.101	dm7020hd.localdomain		dm7020hd
2a02:1810:4d00:9202:f838:44a4:88b5:576e 	raspberry57.localdomain raspberry57

script:

#!/bin/bash

while read client
do
	IP="$(echo $client | cut --delimiter " " --fields 1)"
	sudo sqlite3 /etc/pihole/gravity.db "insert or ignore into client (ip) values (\"$IP\");"
	done < /etc/localdns.list

I would like to be able to run an SQLite update query, based on the comment field. My IPv6 addresses changes whenever my provider resets my modem, running an update query on a device name (the optional comment) is easier than running this on the previous IP address.

An additional IP type type (v4 or v6) might make the update query even simpeler, when using the same comment (device name - see example table).

@DL6ER: optionally, if the developers don't like this request, would it be harmful to pihole-FTL, if I just add the additional fields to the table myself (unsupported database modification)?

3

The best approach may be to try this and see if anything breaks.

5

I agree, just try it.

FTL should not have any issues with it as I was always very specific about database actions, intentionally not assuming a particular database structure (except, of course, that no fields are missing). Additional fields should not hurt FTL.

However, additional fields would likely hurt gravity as the new gravity database (the TEMP one) will be created without any extra fields and copying the data from left to right (old to new) will very likely cause issues as the number of table rows will be different. So you'd also need to adapt

However, I agree that we might want to add this field and only if it is to have the same structure as with domains and adlists. There is no real reason to have this one table look differently. This would also add the "created" and "last modified" timestamps.

6

I just setup a new beta5 system (pihole v4.3.2 -> beta5) because I was experiencing some strange behavior and wanted to ensure I wasn't reporting issues, that occurred only on my system.

Because getting support, or reporting issues on a modified system, isn't really appreciated by the developers, I'm currently NOT going to try this experiment (modifying the database). I pissed of some developers more than enough, I think they start to hate me, so I will be silent for a while.

Thanks anyway for all the great work, updates and modifications, you all have been adding over the last week.

Have a nice day.

8

The code for this has been written and tested.

Pi-hole core:

Web:

https://github.com/pi-hole/AdminLTE/tree/new/client_comments

There is no PR yet for the latter as we're waiting on

to get reviewed and merged, first (the code is based upon this branch to reduce work).

9

I just upgraded to v11 of the database, using your script.

fields 'date_added' and 'date_modified' are set to 'not null' 'no' in the client table.
In the adlist table, they are set to 'not null' 'yes'.

When adding a client (scripted), the date fields are NOT updated.

while read client
do
	IP="$(echo $client | cut --delimiter " " --fields 1)"
	COMMENT="$(echo $client | grep -o '[^ ]*$')"
	sudo sqlite3 /etc/pihole/gravity.db "insert or ignore into client (ip, comment) values (\"$IP\", \"$COMMENT\");"
	done < /etc/localdns.list

I assumed the date fields would be automatically updated. correct?

10

@DL6ER: just update the database to v11

when running pihole -g, after the upgrade, I got the following error:

 [i] Swapping databases...
  [✗] Unable to copy data from /etc/pihole/gravity.db to /etc/pihole/gravity_temp.db
  Error: near line 18: table client has 2 columns but 5 values were supplied
  [✓] Flushing DNS cache
12

@jpgpi250 No, your assumption was wrong. Updating the database also is not sufficient. The fields added in there are only mock fields, they are not expected to update. In contrast, they will just be present for a tiny amount of time and will be replaced by the new gravity.db created by gravity. It is not supported to upgrade the database outside of a pihole -g run.

This shows that you're still on the release/v5.0 branch. However, my fix also included some modifications to other files in the entire gravity process. You cannot mix old and new code.

13

Appologies, I've waisted your time, will wait for the changes to be pushed.

14

If you want to continue from here, just check out

pihole checkout core new/client_comments
15

@PromoFaux

I noticed you just approved 1140, mentioned in this topic earlier.

Any reason why you didn't approve 3107, this appears to be pretty straightforward.

Apologies for my impatience, just trying to keep track of the new and improved features of pihole beta5.

16

Because, dear heart, I will get to it when I get to it :slight_smile:

(I had 10 mins to review some PRs last night, and I just did the web ones... )

4 Likes
18

Stop pinging developers, I've asked you privately and now I'm telling you publicly. Don't do it.

3 Likes