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

(Re)Add support for multiple Pi-Holes #27569

Merged
merged 20 commits into from
Dec 12, 2019
Merged

(Re)Add support for multiple Pi-Holes #27569

merged 20 commits into from
Dec 12, 2019

Conversation

johnluetke
Copy link
Contributor

@johnluetke johnluetke commented Oct 12, 2019 •
edited
Loading

Breaking Change:

The default value for host was removed, and now must be specified manually.

Description:

(Re)Adds support for configuring multiple Pi-holes. This was inadvertently removed in #25837

Related issue (if applicable): fixes #26700

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10758

Example entry for configuration.yaml (if applicable):

pi_hole:
  - host: pi.hole
    ssl: false
  - host: backup.pi.hole

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@johnluetke johnluetke requested a review from fabaff as a code owner October 12, 2019 23:23
@probot-home-assistant
Copy link

Hey there @fabaff, mind taking a look at this pull request as its been labeled with a integration (pi_hole) you are listed as a codeowner for? Thanks!

@johnluetke johnluetke mentioned this pull request Oct 12, 2019
Merged
2 tasks
@johnluetke johnluetke changed the title (Re)Add support multiple Pi-Holes (Re)Add support for multiple Pi-Holes Oct 12, 2019
@johnluetke
Copy link
Contributor Author

pinging @fabaff for a review

@johnluetke johnluetke mentioned this pull request Nov 4, 2019
Closed
@johnluetke
Copy link
Contributor Author

Re-pinging @fabaff for review

MartinHjelmare
MartinHjelmare requested changes Nov 4, 2019
View reviewed changes
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Show resolved Hide resolved
homeassistant/components/pi_hole/sensor.py Outdated Show resolved Hide resolved
@johnluetke johnluetke force-pushed the multiple-piholes branch 3 times, most recently from 5258f1d to 4f43b20 Compare November 10, 2019 20:27
MartinHjelmare
MartinHjelmare reviewed Nov 10, 2019
View reviewed changes
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/__init__.py Outdated Show resolved Hide resolved
johnluetke
johnluetke commented Nov 16, 2019
View reviewed changes
homeassistant/components/pi_hole/__init__.py
conf[CONF_NAME],
conf[CONF_SLUG],
conf[CONF_HOST],
names.get(conf[CONF_NAME], slugs[conf[CONF_SLUG]]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the default value functionality of get here, to fallback onto identifying the dupe via slug if it was not triggered on name.

MartinHjelmare
MartinHjelmare reviewed Nov 16, 2019
View reviewed changes
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
MartinHjelmare
MartinHjelmare reviewed Nov 17, 2019
View reviewed changes
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
MartinHjelmare
MartinHjelmare reviewed Nov 23, 2019
View reviewed changes
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Outdated Show resolved Hide resolved
@johnluetke
Ensure each Pi-hole has a unique name
091c181
@johnluetke
Update services to handle multiple Pi-holes
254164b
@johnluetke
Update tests for multiple configurations
ecba3f4
@johnluetke
Refactor tests to support service testing
4241cd4
@johnluetke
Fix else-raise per pyliunt
8fc853e
@johnluetke
Per code review, add all entities in a single call
97b2e19
@johnluetke
Per code review, add the default name as default.
4fb5b5f
@johnluetke
Per code review, add cv.ensure_list to prevent breaking change
1a7aed4
@johnluetke
Per code review, move name validation to schema
a55dcdd
@johnluetke
Remove default name
fa275b6
@johnluetke
Per code review, validate api_key in schema definition
ddae0fe
@johnluetke
Per code review, rename variables
d002897
@johnluetke
Per code review, use list comprehension
97e11ba
@johnluetke
Ensure unique slug names in config validation
8d9dc64
@johnluetke
Per code review, refactor to CoroutineMock
5148a90
@johnluetke
Fix adding sensor entities
e48bd54
@johnluetke
Per code review, refactor mock function creation
28b9fce
@johnluetke
Copy link
Contributor Author

johnluetke commented Nov 23, 2019 •
edited
Loading

@MartinHjelmare FYI, it appears that the build pipeline is broken due to a missing dependency , blocking a successful build of this PR. I attempted a repush, as well as running gen_requirements_all, but nothing was regenerated.

Seems to have self-resolved.

MartinHjelmare
MartinHjelmare approved these changes Nov 28, 2019
View reviewed changes
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare
Copy link
Member

There's still a breaking change since we made host config item required. Please mention this in the breaking change paragraph.

@johnluetke
Copy link
Contributor Author

@MartinHjelmare all set.

@MartinHjelmare MartinHjelmare merged commit 7c42f4b into home-assistant:dev Dec 12, 2019
@lock lock bot locked and limited conversation to collaborators Dec 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@fabaff fabaff Awaiting requested review from fabaff

Assignees

@fabaff fabaff

Labels
breaking-change by-code-owner cla-signed Hacktoberfest has-tests integration: pi_hole
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor pihole integration
4 participants
@johnluetke @MartinHjelmare @fabaff @homeassistant