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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Deprecate auto target all for services and introduce entity_id: * #19006

Merged
merged 2 commits into from Dec 13, 2018

Conversation

Projects
None yet
5 participants
@balloob
Copy link
Member

balloob commented Dec 4, 2018

Description:

If entity_id is omitted, we currently target all entities. That's pretty dangerous and for any installation that has more than 1 room in Home Assistant, useless and sometimes dangerous (like triggering all automations 馃檲).

This PR:

  • introduces a new wildcard for entity_id to indicate you want to target all entity_id: "all".
  • prints a warning if old method is used with target all

We should not add support in the future to add regex or partial wildcards as that will remove the ability to statically analyse the config. And this is not the PR to discuss this either, related comments will be marked as off topic.

Pull request in home-assistant.io with documentation (if applicable): TODO

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.

If the code does not interact with devices:

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

@balloob balloob requested review from fabaff and home-assistant/core as code owners Dec 4, 2018

@wafflebot wafflebot bot added the in progress label Dec 4, 2018

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 4, 2018

Is step two, after deprecation period, to make entity_id required in all service schemas that target entity component services?

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 4, 2018

Yeah

@marchingphoenix

This comment has been minimized.

Copy link
Contributor

marchingphoenix commented Dec 5, 2018

Instead of * could we do ALL? Would remove the temptation of using it like a wildcard.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 6, 2018

Updated to match on word "all"

balloob added some commits Dec 4, 2018

@balloob balloob force-pushed the deprecate-auto-target-all branch from b4984c0 to 6bfe294 Dec 6, 2018

@balloob balloob merged commit 8ea0a8d into dev Dec 13, 2018

6 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override 鈥 see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 92.929%
Details

@wafflebot wafflebot bot removed the in progress label Dec 13, 2018

@delete-merged-branch delete-merged-branch bot deleted the deprecate-auto-target-all branch Dec 13, 2018

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

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

@WedHumpDay

This comment has been minimized.

Copy link

WedHumpDay commented Jan 10, 2019

Maybe outline what a end user should be looking for. Not all users are savvy and just search and tweak existing examples.

Here is an example automation script created and its generating the warning:

Warning Message
2019-01-10 13:59:00 WARNING (MainThread) [homeassistant.helpers.service] Not passing an entity ID to a service to target all entities is deprecated. Use instead: entity_id: "*"

Automation Script

- id: '1539091617090'
  alias: '----Test Dry Contact Sensor----'
  trigger:
  - entity_id: binary_sensor.visonic_mct340_e_0b110ced_1_1280
    from: 'off'
    platform: state
    to: 'on'
  condition: []
  action:
  - data:
      message: Test Door is Opened
      target:
      - Test_Cell
      title: ''
    service: notify.cell_phones
  - alias: ''
    data:
      volume_level: '.50'
    service: media_player.volume_set
  - alias: ''
    data:
      entity_id: media_player.living_room_2
      message: Test door is opened
    service: tts.google_say
  - timeout: '3'
    wait_template: ''
  - data:
      volume_level: '.25'
    service: media_player.volume_set
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 10, 2019

You're setting the volume of all your media players at the same time. You need to specify entity_id: all if you really want that.

Also please do not post in merged PRs.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Jan 10, 2019

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