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

IGD review fixes #17400

Merged
merged 7 commits into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@StevenLooman
Contributor

StevenLooman commented Oct 13, 2018

Description:

Changes after review by @balloob

Related issue (if applicable): fixes #16300

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

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

@wafflebot wafflebot bot added the in progress label Oct 13, 2018

@dgomes dgomes changed the title from Igd review fixes to UPnP:IGD review fixes Oct 13, 2018

@fabaff fabaff changed the title from UPnP:IGD review fixes to IGD review fixes Oct 13, 2018

@@ -31,7 +30,7 @@
REQUIREMENTS = ['async-upnp-client==0.12.4']
DEPENDENCIES = ['http']
DEPENDENCIES = ['http', 'discovery']

This comment has been minimized.

@balloob

balloob Oct 15, 2018

Member

I'm actually thinking, why is it that we need both http and discovery to begin with?

This comment has been minimized.

@StevenLooman

StevenLooman Oct 15, 2018

Contributor

discovery is required as the device is detected via upnp/M-SEARCH.
http is required to determine the port home assistant is listening on (really only used the port mapping is enabled.)

This comment has been minimized.

@balloob

balloob Oct 16, 2018

Member

But that is not needed for running this component, the SSDP response is stored in the config entry.

I think that we should remove both http and discovery, because marking it as dependencies means that we're setting it up.

If port mapping is enabled but http is not enabled: don't setup port mapping.
Not requiring discovery: nothing breaks?.

This comment has been minimized.

@StevenLooman

StevenLooman Oct 16, 2018

Contributor

If we get a SSDP response discovery is enabled already. The original component already (only) worked with discovery, but used its own library for it. Now the component is using more existing libraries, such as netdisco and async_upnp_client (used also by the dlna_dmr-component).

The 'externals' (config, workings from the perspective of the user, etc) are mostly kept the same compared to the original upnp component. Thus, depending on the http component for getting the hass-port for port mapping. discovery is required as the user has (as before) no way to manually enter a SSDP description URL. Learned from the dlna_dmr component, finding this URL is hard for the average user and discovery is preferable.

I get your point and I'm fine with not requiring these, but then there should be:

  • a message in the logs clearly stating that discovery is required otherwise it will not work (as before)
  • a message in the docs clearly stating that discovery is required otherwise it will not work

This comment has been minimized.

@balloob

balloob Oct 18, 2018

Member

Philips Hue will, during their config flow, do their own discovery to find the device. The upnp component should do the same.

This comment has been minimized.

@StevenLooman

StevenLooman Oct 20, 2018

Contributor

That what is the purpose of netdisco/discovery if every component has to do its own discovery? This feels like unnecessary duplication of functionality, and hence, more suspect to bugs.

This comment has been minimized.

@balloob

balloob Oct 21, 2018

Member

It serves as purpose to find config entries. However, manual config entries will have to rely on their own scanning logic.

@balloob

This comment has been minimized.

Member

balloob commented Oct 16, 2018

btw I notice that you're calling get_local_ip inside an async context in upnp/device.py. That call does I/O.

@StevenLooman

This comment has been minimized.

Contributor

StevenLooman commented Oct 16, 2018

btw I notice that you're calling get_local_ip inside an async context in upnp/device.py. That call does I/O.

Creating port mappings does I/O as well. Where do you suggest to do it?

@balloob

This comment has been minimized.

Member

balloob commented Oct 18, 2018

Outside the event loop. No I/O allowed inside it.

ip = await hass.async_add_executor_job(get_local_ip)

@StevenLooman StevenLooman force-pushed the StevenLooman:igd_review_fixes branch from 192cf0f to a7b1f2c Oct 20, 2018

@@ -31,7 +31,7 @@
REQUIREMENTS = ['async-upnp-client==0.12.7']
DEPENDENCIES = ['http']
DEPENDENCIES = ['discovery']

This comment has been minimized.

@balloob

balloob Oct 21, 2018

Member

We should not depend on discovery.

@StevenLooman

This comment has been minimized.

Contributor

StevenLooman commented Oct 23, 2018

Work has been slow, but will pick this up soon.

@balloob balloob merged commit fc8af22 into home-assistant:dev Oct 23, 2018

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 93.583%
Details

@wafflebot wafflebot bot removed the in progress label Oct 23, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 23, 2018

It's already ok, will merge it. I removed the discovery dependency.

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

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