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

Correct expose_by_default interaction with expose_domains #17745

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
5 participants
@glentakahashi
Contributor

glentakahashi commented Oct 24, 2018

Home Assistant release with the issue:

0.80.3

Last working Home Assistant release (if known):

Operating environment (Hass.io/Docker/Windows/etc.):

Component/platform:

https://www.home-assistant.io/components/google_assistant/#expose_by_default

Description of problem:
Based on the documentation here: https://www.home-assistant.io/components/google_assistant/#expose_by_default it seems that expose_by_default means all devices should be exposed unless explicitly set to false, and that regardless if this is set domains in exposed_domains should be exposed.

Problem-relevant configuration.yaml entries and (fill out even if it seems unimportant):

google_assistant:
  project_id: xxx
  api_key: xxx
  expose_by_default: false
  exposed_domains:
    - switch
    - light

Traceback (if applicable):


Additional information:

Correct expose_by_default interaction with expose_domains
Based on the documentation here: https://www.home-assistant.io/components/google_assistant/#expose_by_default it seems that expose_by_default means all devices should be exposed unless explicitly set to false, and that regardless if this is set domains in exposed_domains should be exposed.
@homeassistant

This comment has been minimized.

homeassistant commented Oct 24, 2018

Hi @glentakahashi,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 25, 2018

Unclear to me whether this is actually needed or not, but regardless it should be targeted at the dev branch not master

@glentakahashi glentakahashi changed the base branch from master to dev Oct 25, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 26, 2018

It's an and check so that people don't expose domains that we don't support.

@glentakahashi

This comment has been minimized.

Contributor

glentakahashi commented Oct 26, 2018

That doesn't make sense to me. The documentation for expose_by_default says:

Expose devices in all supported domains by default. If set to false, you need to either expose domains or add the expose configuration option to each entity in entity_config and set it to true.

I'm understanding this as, an entity will be exposed if:

  1. expose_by_default is true and is not manually excluded
  2. expose_by_default is false and either:
  3. the domain is exposed by exposed_domains
  4. the device is manually exposed in the entity_config

Right now (using and) the logic is coded to expose things if:

  1. expose_by_default is true, the domain is in exposed_domains, and is not manually excluded
  2. expose_by_default is false and the device is manually exposed in entity_config

So which one is right here? If the docs are wrong I can make a PR to fix those, but in my opinion the docs behavior is the more intuitive one and this PR will adjust the code to work like that.

@balloob

This comment has been minimized.

Member

balloob commented Nov 1, 2018

If a domain is not in expose_by_default, it's not supported. So no need to route entities through the GA flow and then make it generate a ton of warnings.

@balloob balloob closed this Nov 1, 2018

@wafflebot wafflebot bot removed the in progress label Nov 1, 2018

@glentakahashi

This comment has been minimized.

Contributor

glentakahashi commented Nov 1, 2018

@balloob I still believe either the documentation or code needs changing here. The documentation doesn't match up with how the code works. Take this example. Let's say I have three devices in three domains, one of which is unsupported:

Devices:

  • bedroom_light - LIGHT
  • bedroom_switch - SWITCH
  • bedroom_test - TEST

Current behavior:

expose_by_default: true

LIGHT, SWITCH are exposed

expose_by_default: true
exposed_domains: LIGHT

only LIGHT is exposed

expose_by_default: false
exposed_domains: LIGHT

no devices are exposed

expose_by_default: false
exposed_domains: LIGHT
devices:
  bedroom_switch:
    expose: true

only SWITCH is exposed

Using OR behavior:

expose_by_default: true

LIGHT, SWITCH are exposed

expose_by_default: true
exposed_domains: LIGHT

LIGHT, SWITCH are exposed

expose_by_default: false
exposed_domains: LIGHT

LIGHT is exposed

expose_by_default: false
exposed_domains: LIGHT
devices:
  bedroom_switch:
    expose: true

LIGHT, SWITCH are exposed

To me, use cases #3 and #4 are what seem wrong in the current behavior. I see someone updated the documentation today to make it clear this is how it works (home-assistant/home-assistant.io#7292) but I think the config can still be improved.

In my opinion, if not the OR behavior above there are a few other alternatives:

Lastly, currently the code itself doesn't protect against unsupported domains in GA either. The only line of code that uses the filtered set of domains is in the default configuration above. To actually support this, I think instead in const.py there should be a list of supported_domains and that is intersected with the provided exposed_domains and device sets. Right now, if I were to add a exposed_domains: TEST, the code would actually expose the TEST domain to GA.

Sorry for the long post, but just want to clear this up.

@balloob balloob reopened this Nov 2, 2018

@wafflebot wafflebot bot added the in progress label Nov 2, 2018

@balloob

This comment has been minimized.

Member

balloob commented Nov 2, 2018

I'm sorry, looks like you're right.

@balloob balloob merged commit 34d7758 into home-assistant:dev Nov 6, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot All contributors have signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on patch-1 at 93.064%
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@glentakahashi glentakahashi deleted the glentakahashi:patch-1 branch Nov 12, 2018

glentakahashi added a commit to glentakahashi/home-assistant.io that referenced this pull request Nov 12, 2018

Revert home-assistant#7292
Now that home-assistant/home-assistant#17745 has been merged, the original documentation is back to being correct so the docs changes should be reverted once the version with home-assistant/home-assistant#17745 has been released.

@glentakahashi glentakahashi referenced this pull request Nov 12, 2018

Merged

Revert #7292 #7479

2 of 2 tasks complete

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Correct expose_by_default interaction with expose_domains (home-assis…
…tant#17745)

Based on the documentation here: https://www.home-assistant.io/components/google_assistant/#expose_by_default it seems that expose_by_default means all devices should be exposed unless explicitly set to false, and that regardless if this is set domains in exposed_domains should be exposed.

frenck added a commit to home-assistant/home-assistant.io that referenced this pull request Nov 19, 2018

Revert #7292 (#7479)
* Revert #7292

Now that home-assistant/home-assistant#17745 has been merged, the original documentation is back to being correct so the docs changes should be reverted once the version with home-assistant/home-assistant#17745 has been released.

* Add missing quote

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@balloob

This comment has been minimized.

Member

balloob commented Dec 4, 2018

we need to revert this change because it's causing issues #18856

@glentakahashi

This comment has been minimized.

Contributor

glentakahashi commented Dec 5, 2018

oh no! Sorry about the issues that this is causing. Would you be okay with me putting out another PR with better functionality + testing after you revert?

balloob added a commit that referenced this pull request Dec 6, 2018

@balloob balloob referenced this pull request Dec 6, 2018

Merged

Revert #17745 #19064

@balloob

This comment has been minimized.

Member

balloob commented Dec 6, 2018

If we do so, it should be done as a non breaking change.

balloob added a commit that referenced this pull request Dec 6, 2018

balloob added a commit that referenced this pull request Dec 7, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 7, 2018

Merge branch 'dev' into current
* dev: (52 commits)
  Set lock status correctly for Schlage BE469 Z-Wave locks (home-assistant#18737)
  Upgrade Tibber lib (home-assistant#19098)
  Bump skybellpy version to fix api issue (home-assistant#19100)
  Automatically detect if ipv4/ipv6 is used for cert_expiry (home-assistant#18916)
  Upgrade pyatv to 0.3.12 (home-assistant#19085)
  Bump lakeside requirement to support more Eufy devices (home-assistant#19080)
  Set directv unavailable state when errors returned for longer then a minute (home-assistant#19014)
  Updated frontend to 20181207.0
  Force refresh Lovelace (home-assistant#19073)
  Upgrade aiolifx to 0.6.7 (home-assistant#19077)
  Fix missing colorTemperatureInKelvin from Alexa responses (home-assistant#19069)
  Add CM17A support (home-assistant#19041)
  Upgrade pylint to 2.2.2 (home-assistant#18750)
  Revert home-assistant#17745 (home-assistant#19064)
  Bumped version to 0.85.0.dev0
  Add support for more Tibber Pulse data (home-assistant#19033)
  Update locationsharinglib to 3.0.9 (home-assistant#19045)
  Implemented unique ID support for Fibaro hub integration (home-assistant#19055)
  Update pyhomematic to 0.1.53 (home-assistant#19056)
  Fix saving YAML as JSON with empty array (home-assistant#19057)
  ...

@balloob balloob referenced this pull request Dec 12, 2018

Merged

0.84 #19215

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