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.

Sign up for GitHub

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

Jump to bottom

[pihole] New binding PiHole #16627

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

[pihole] New binding PiHole #16627

wants to merge 26 commits into from
+1,406 −0

Conversation

magx2
Copy link
Contributor

@magx2 magx2 commented Apr 7, 2024

Hey,

I'm finishing writing readme but the code can be reviewed.

Cheers

@magx2 magx2 requested a review from a team as a code owner April 7, 2024 17:52
@holgerfriedrich holgerfriedrich added the new binding If someone has started to work on a binding. For a new binding PR. label Apr 7, 2024
@jlaur jlaur changed the title New binding PiHole [pihole] New binding PiHole Apr 7, 2024
@magx2 magx2 requested a review from clinique April 12, 2024 06:51
clinique
clinique requested changes Apr 12, 2024
View reviewed changes
bundles/org.openhab.binding.pihole/README.md Show resolved Hide resolved
bundles/org.openhab.binding.pihole/pom.xml

<dependencies>
<dependency>
<groupId>ch.qos.logback</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this dependency required ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To run unit tests and have logging

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at how it's done in other bindings . Tests should not be in the binding pom apparently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are talking about integration tests not unit https://www.openhab.org/docs/developer/tests.html#unit-tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't check details, but might help:

openhab-addons/bundles/org.openhab.binding.hdpowerview/pom.xml

Lines 23 to 30 in a9f7e0f

<dependencies>
<dependency>
<groupId>jcifs</groupId>
<artifactId>jcifs</artifactId>
<version>1.3.17</version>
<scope>compile</scope>
</dependency>
</dependencies>

openhab-addons/bundles/org.openhab.binding.hdpowerview/src/test/java/org/openhab/binding/hdpowerview/internal/AutomationChannelBuilderTest.java

Lines 37 to 38 in a9f7e0f

import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You both see that this dep is in test scope, right?

 <dependencies>
    <dependency>
      <groupId>ch.qos.logback</groupId>
      <artifactId>logback-classic</artifactId>
      <version>1.2.3</version>
      <scope>test</scope> <!-- here -->
    </dependency>

....openhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/PiHoleHandler.java Outdated Show resolved Hide resolved
....openhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/PiHoleHandler.java Show resolved Hide resolved
....openhab.binding.pihole/src/main/java/org/openhab/binding/pihole/internal/PiHoleHandler.java Show resolved Hide resolved
...binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/JettyAdminService.java Outdated Show resolved Hide resolved
...binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/JettyAdminService.java Outdated Show resolved Hide resolved
...binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/JettyAdminService.java Outdated Show resolved Hide resolved
...binding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/JettyAdminService.java Outdated Show resolved Hide resolved
...nding.pihole/src/main/java/org/openhab/binding/pihole/internal/rest/model/DnsStatistics.java Show resolved Hide resolved
@magx2 magx2 requested a review from clinique April 12, 2024 13:45
@magx2 magx2 force-pushed the pihole/master branch 3 times, most recently from e0c41a3 to 80109b4 Compare April 14, 2024 12:34
@magx2
Copy link
Contributor Author

magx2 commented Apr 15, 2024

@clinique any update?

@miloit
Copy link
Contributor

miloit commented Apr 17, 2024

Hello, binding is only for pihole v5 ? Also V6?

@magx2
Copy link
Contributor Author

magx2 commented Apr 17, 2024

Hello, binding is only for pihole v5 ? Also V6?

Is there v6? I see only v5 in releases.

Personally I was testing it with:

Docker Tag 2024.02.2 · Pi-hole v5.17.3 FTL v5.25.1 Web Interface v5.21

magx2 added 16 commits April 24, 2024 21:00
@magx2
Init PiHole binding
d854341 
God save the King!

Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Set up basic PiHole bundle that shows summary from API
fc56ece 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Implement PiHoleActions.java
8240434 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add disable/enable channel
7850835 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add i8n
2c5b690 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Format 😭😭😭
406a998 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add header
ebeed7f 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add author
c42ef6d 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Fixe checkstyles and other warnings
3e3ab26 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add codeowners
00b9bcd 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Remove not used log
2237b20 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Use proper error handling
46fe374 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Proper mapping of percentage
4ea8ffd 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
refresh after enabling/disabling blocking
be1c288 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Use set of
b0372b1 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Use record instead of pojos
f5f005c 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
magx2 added 10 commits April 24, 2024 21:00
@magx2
Change channel names
330e5cd 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add disable blocking for infinity
39a2be2 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Add README.md
9e95423 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
formatted
949cb82 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
static gson
52900d3 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
non static logger
d1fbb1e 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
check if refreshIntervalSeconds is valid before creating admin service
d78bb1e 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
star imports
dfce7da 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
Proper indents
818be02 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@magx2
remove bad enter
2e9ee4f 
Signed-off-by: Martin Grześlowski <martin.grzeslowski@gmail.com>
@miloit
Copy link
Contributor

miloit commented May 21, 2024

Here you can test....
https://discourse.pi-hole.net/t/pi-hole-v6-beta-testing/65413

No official release date....but I would go for the pihole v5 integration first and than it's easier to implement v6 api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur left review comments

@clinique clinique Awaiting requested review from clinique

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@magx2 @miloit @clinique @jlaur @holgerfriedrich