Add OPNSense device tracker by mtreinish · Pull Request #26834 · home-assistant/core

mtreinish · 2019-09-22T19:51:29Z

Description:

This commit adds a new component for using an OPNSense router as a
device tracker. It uses pyopnsense to query the api to look at the
arptable for a list of devices on the network.

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

Example entry for `configuration.yaml` (if applicable):

opnsense:
   url: http://$router_ip/api
   api_key: api_key
   api_secret: api_secret

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:

Documentation added/updated in home-assistant.io

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.

fabaff · 2019-09-23T05:46:33Z

+    "pyopnsense==0.2.0"
+  ],
+  "dependencies": [],
+  "codeowners": []


Please add yourself as code owner.

Oh, I just assumed I didn't need to because I'm not listed as a codeowner on any of the components I've contributed in the past. Figured it was a requirement. Fixed in: 914fce5

mtreinish · 2019-10-02T12:37:53Z

It looks like the linter job failed because of a network issue trying to talk to pypi: https://dev.azure.com/home-assistant/Home%20Assistant/_build/results?buildId=10835&view=logs&j=4e214fed-f310-53fa-0018-b38e7beb405b&t=065ca18d-6864-5af3-aac6-258b23c5c37b&l=87

MartinHjelmare · 2019-10-03T15:50:34Z

+    )
+    try:
+        interfaces_client.get_arp()
+    except Exception:  # pylint: disable=broad-except


Please catch a specific exception raised by the library on credentials or connection failure.

It's good if the library raises different exceptions for different type of problems.

It depends on the failure mode, the library will raise a pyopnsense.exceptions.APIException if the api returns an error, because of invalid credentials for example. But if there is a connection issue or something else that happens at a lower level like requests or urllib3 that will get raised verbatim. Which is why I used a broad catch here. It seemed like it would be better to catch any exception and log it rather than raise an unhandled exception from a lower layer.

Preferably the library should catch RequestException and raise a library specific error.

MartinHjelmare · 2019-10-03T15:54:25Z

If we need the library in the tests we need to add the library to the list in script/gen_requirements_all.py.

https://github.com/home-assistant/home-assistant/blob/bb45bdd8dd937a9fbbb21def1da93299498a9290/script/gen_requirements_all.py#L44

frenck · 2019-11-20T08:51:31Z

Hi @mtreinish!

Looks like this PR is pretty close to the finish 🥇
Are you planning to continue to work on this PR?

At this point, I would recommend rebasing the PR onto the latest dev branch, regenerate the requirements, and go from there.

Looking forward to it! 👍

springstan · 2019-12-10T07:17:56Z

Hello @mtreinish the failed tests seem to be unrelated to this PR, that is why I would recommend you to rebase onto the latest dev branch again. This way the tests have to rerun and hopefully pass 😅 so this can be finally merged ✌

This commit adds a new component for using an OPNSense router as a device tracker. It uses pyopnsense to query the api to look at the arptable for a list of devices on the network.

Co-Authored-By: Fabian Affolter <mail@fabian-affolter.ch>

This commit updates several issues found in the module during code review.

Co-Authored-By: Martin Hjelmare <marhje52@kth.se>

This commit fixes several issues from review comments, including abandoning all the use of async code. This also completely reworks the tests to be a bit clearer.

This commit adds actual device detection to the unit test for the setup test. A fake api response is added to mocks for both api clients so that they will register devices as expected and asserts are added for that. The pyopnsense import is moved from the module level to be runtime in the class. This was done because it was the only way to make the MockDependency() call work as expected.

This commit updates the connection logic to return false if we're unable to connect to the configured OPNsense API endpoint for any reason. Previously we would not catch if an endpoint was incorrectly configured until we first tried to use it. In this case it would raise an unhandled exception. To handle this more gracefully this adds an api call early in the setup and catches any exception raised by that so we can return False to indicate the setup failed.

Since opening the PR originally yet another lint/style checker was added which failed the PR in CI. This commit makes the adjustments to have this pass the additional tool's checks.

This commit adds documentation for the opnsense device tracker which is being added in home-assistant/core#26834.

fixed

homeassistant added the cla-signed label Sep 22, 2019

probot-home-assistant Bot added has-tests new-integration integration: opnsense labels Sep 22, 2019

mtreinish mentioned this pull request Sep 22, 2019

Add documentation for new opnsense integration home-assistant/home-assistant.io#10427

Merged

2 tasks

fabaff reviewed Sep 23, 2019

View reviewed changes

MartinHjelmare previously requested changes Sep 23, 2019

View reviewed changes

MartinHjelmare reviewed Sep 23, 2019

View reviewed changes

Comment thread tests/components/opnsense/test_device_tracker.py Outdated

MartinHjelmare reviewed Oct 2, 2019

View reviewed changes

Comment thread tests/components/opnsense/test_device_tracker.py Outdated

MartinHjelmare reviewed Oct 2, 2019

View reviewed changes

Comment thread homeassistant/components/opnsense/__init__.py Outdated

Comment thread tests/components/opnsense/test_device_tracker.py Outdated

MartinHjelmare reviewed Oct 3, 2019

View reviewed changes

mtreinish force-pushed the opnsense-new branch from 87ebf81 to baf01a1 Compare December 10, 2019 11:51

mtreinish and others added 14 commits December 22, 2019 15:20

Add OPNSense device_tracker

de55b05

This commit adds a new component for using an OPNSense router as a device tracker. It uses pyopnsense to query the api to look at the arptable for a list of devices on the network.

Run black formatting locally to appease azure

1a72bb0

Apply suggestions from code review

64a624c

Co-Authored-By: Fabian Affolter <mail@fabian-affolter.ch>

Update homeassistant/components/opnsense/__init__.py

dfa6ba0

Co-Authored-By: Fabian Affolter <mail@fabian-affolter.ch>

Fix issues identified during code review

b93275b

This commit updates several issues found in the module during code review.

Update CODEOWNERS for recent changes

3dfa4ec

Fix lint

a4e946c

Apply suggestions from code review

fca5838

Co-Authored-By: Martin Hjelmare <marhje52@kth.se>

More fixes from review comments

ef3ed62

This commit fixes several issues from review comments, including abandoning all the use of async code. This also completely reworks the tests to be a bit clearer.

Revert tests to previous format

18d4f04

Rerun black

60339b4

Fix lint

eb4a851

Move import back to module level

c8bc10f

mtreinish added 4 commits December 22, 2019 15:20

Update tests

ed48751

Add pyopnsense to test requirements

595c26c

Rerun gen_requirements script

5b368c6

mtreinish force-pushed the opnsense-new branch from baf01a1 to 5b368c6 Compare December 22, 2019 20:21

Fix failing isort lint job step

9309a6a

Since opening the PR originally yet another lint/style checker was added which failed the PR in CI. This commit makes the adjustments to have this pass the additional tool's checks.

frenck pushed a commit to mtreinish/home-assistant.io that referenced this pull request Jan 9, 2020

Add documentation for new opnsense component

71129a7

This commit adds documentation for the opnsense device tracker which is being added in home-assistant/core#26834.

Fix comment

84bfcdd

pvizeli approved these changes Jan 29, 2020

View reviewed changes

Update manifest.json

5936635

pvizeli merged commit 85dbf1f into home-assistant:dev Jan 29, 2020

mtreinish deleted the opnsense-new branch January 29, 2020 15:54

lock Bot locked and limited conversation to collaborators Jan 30, 2020

Uh oh!

Conversation

mtreinish commented Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Uh oh!

Uh oh!

fabaff Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

mtreinish Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtreinish commented Oct 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinHjelmare Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

mtreinish Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Oct 3, 2019

Uh oh!

frenck commented Nov 20, 2019

Uh oh!

springstan commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mtreinish commented Sep 22, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):