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

Refactor pihole integration #25837

Merged
merged 11 commits into from Sep 3, 2019

Conversation

@johnluetke
Copy link
Contributor

commented Aug 10, 2019

Breaking Change:

This change requires existing configurations for the pi_hole integration to be moved from sensor platform key to a pi_hole top level key.

  • monitored_conditions is removed
  • pi_hole configuration block defines host, port, location and ssl options

configuration.yaml before this change:

sensor:
  - platform: pi_hole
    host: pi.hole
    monitored_conditions:
      - ads_blocked_today
      - ads_percentage_today
      - dns_queries_today
      - domains_being_blocked
      - queries_cached
      - queries_forwarded
      - unique_clients
      - unique_domains

configuration.yaml after this change:

pi_hole:
  host: pi.hole

Description:

Refactors the pi_hole component from a single sensor to an integration with a sensor platform. This allows services, switches, and possibly other entities to be added to the integration in a future pull request.

Pull request with documentation for home-assistant.io: home-assistant/home-assistant.io#10097

Example entry for configuration.yaml (if applicable):

pi_hole:
  host: 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.
@probot-home-assistant

This comment has been minimized.

Copy link

commented Aug 10, 2019

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 added a commit to johnluetke/home-assistant.io that referenced this pull request Aug 10, 2019
@johnluetke johnluetke referenced this pull request Aug 10, 2019
2 of 2 tasks complete
@johnluetke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Notes for reviewer: This change lays the groundwork for future functionality i intend to add to the integration shortly after approval. This change itself introduces no new functionality to the integration.

@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch 5 times, most recently from cbed550 to 3093b16 Aug 10, 2019
@MartinHjelmare MartinHjelmare moved this from Needs review to Review in progress in Dev Aug 10, 2019
@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch from 4e78b3e to c35c52e Aug 11, 2019
@pvizeli

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

You can use discovery to setup sensor from component without need a config for this

@johnluetke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

Quick update: I'm still working on working out the tests portion of this change. My day job has been demanding this past week

@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch 3 times, most recently from 634168f to 5921c1c Aug 18, 2019
@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch from a6e2300 to 6e85def Aug 20, 2019
@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch from 6e85def to 1730c39 Aug 20, 2019
@johnluetke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@fabaff @balloob @MartinHjelmare @pvizeli I believe I have addressed all the concerns raised

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/const.py Outdated Show resolved Hide resolved
homeassistant/components/pi_hole/sensor.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_init.py Show resolved Hide resolved
tests/components/pi_hole/test_sensor.py Outdated Show resolved Hide resolved
tests/components/pi_hole/test_sensor.py Outdated Show resolved Hide resolved
johnluetke added 2 commits Aug 31, 2019
@johnluetke

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@MartinHjelmare Requested changes have been made and are ready for re-review

@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch from d3c2303 to cf4c65c Sep 2, 2019
@johnluetke johnluetke force-pushed the johnluetke:refactor-pihole-integration branch from cf4c65c to bf02832 Sep 2, 2019
Dev automation moved this from Review in progress to Reviewer approved Sep 2, 2019
Copy link
Member

left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Can be merged when build passes.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

Please update the config example and breaking change paragraph in the PR description. Please describe briefly what changed and what the user needs to do to cope with the breaking change.

@balloob balloob merged commit 757482e into home-assistant:dev Sep 3, 2019
11 checks passed
11 checks passed
CI Build #20190902.45 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 93.9%)
Details
codecov/project 93.91% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 3, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

We don't require a sensor platform config after this PR. We set up the sensor platform via discovery from the pi_hole component module. I've updated the breaking change paragraph and config examples.

klaasnicolaas added a commit to home-assistant/home-assistant.io that referenced this pull request Sep 4, 2019
@lock lock bot locked and limited conversation to collaborators Sep 5, 2019
@johnluetke johnluetke deleted the johnluetke:refactor-pihole-integration branch Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.