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

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

Merged
merged 2 commits into from
Dec 13, 2018

Conversation

balloob
Copy link
Member

@balloob 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.

@MartinHjelmare
Copy link
Member

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

@balloob
Copy link
Member Author

balloob commented Dec 4, 2018

Yeah

@marchingphoenix
Copy link
Contributor

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

@balloob
Copy link
Member Author

balloob commented Dec 6, 2018

Updated to match on word "all"

@balloob balloob force-pushed the deprecate-auto-target-all branch from b4984c0 to 6bfe294 Compare December 6, 2018 09:17
@balloob balloob merged commit 8ea0a8d into dev Dec 13, 2018
@ghost ghost removed the in progress label Dec 13, 2018
@delete-merged-branch delete-merged-branch bot deleted the deprecate-auto-target-all branch December 13, 2018 09:08
dshokouhi pushed a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018
@balloob balloob mentioned this pull request Jan 10, 2019
@WedHumpDay
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
Copy link
Member Author

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants