-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Move Google Assistant entity config out of customize #11499
Conversation
68c27c2
to
44c091c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I slightly on the fence about the entity_config
change but reading through that issue you all have put way more thought into it than I have so I'm fine with that. It's probably going to be somewhat painful though. Not the place for a feature request but I'd really like to be able to use the same filter across both Alexa and Assistant (without resorting to yaml tricks).
@@ -10,7 +10,8 @@ | |||
import voluptuous as vol | |||
|
|||
from homeassistant.const import ( | |||
EVENT_HOMEASSISTANT_START, CONF_REGION, CONF_MODE) | |||
EVENT_HOMEASSISTANT_START, CONF_REGION, CONF_MODE, CONF_NAME, CONF_TYPE, | |||
CONF_ALIAS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see it remain as "aliases" to make it clear it's an array. Also your doc example should be an array instead of a single value (not sure if that gets auto converted by something but the doc should at least show it can be an array)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and yes, a single value will be converted to a list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went initially for CONF_ALIAS because it's an existing option. However, you're right that it is confusing that it is not plural.
@@ -65,7 +76,7 @@ | |||
vol.Optional(CONF_REGION): str, | |||
vol.Optional(CONF_RELAYER): str, | |||
vol.Optional(CONF_ALEXA): ALEXA_SCHEMA, | |||
vol.Optional(CONF_GOOGLE_ACTIONS): ASSISTANT_SCHEMA, | |||
vol.Optional(CONF_GOOGLE_ACTIONS): GACTIONS_SCHEMA, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this over on the Alexa PR, the rename from Assistant to Actions. What's the motivation there? I ask because I'd originally written this as actions and went with assistant because it felt like that matches what end users will expect the component to be named (only devs see it as Actions on Google).
(Also if I wanted to nitpick I'd say all the assistant->actions renames should be in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's only within the Cloud component. I did not want to change the component name. I called it that way because that seems to be the product name that Google gave their side.
The Cloud component will offer the same filters between both Alexa and Google. I didn't want to introduce a breaking config change to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Will work on an update to the docs today. |
I'd like to know how this breaking change is better than it was with the customize method. ll of my packages contain one purpose which is usually just a single entity and anything that acts upon that object such as automations and naming. by changing this I can no longer have the google assistant name for an entity be changed right there but in another package where i define google_assistant. This means that now if i decide to name the light differently i have to go to one package to update the name of the object and all instance of it for within my HASS server which includes alexa through haaska to now having to hunt down another file JUST TO RENAME THE OBJECT for one service...... this makes no sense to me. The above AFTER code must be able to be in each package file or this is not a solution at all to an "issue" EDIT: i've included an example of an entity from my config to show what i'm talking about. |
Please don't use closed PRs for discussion. Use the forum or discord. |
Without trying to be a smartass or trying to offend anyone:
|
Just echoing what @MartinHjelmare said....let us not use closed PRs for discussion. Devs have already indicated that packages will not be fully integrated (as it is difficult to reload them on runtime and the additional complexity they bring in). It is a small price to be paid for ease of long-term maintenance of the project. Lot of thought goes in before making such breaking changes, as it is bound to upset some users. But, please understand the devs situation, who are responsible for maintaining the long-term health of the project. I, for one, like the fact that the entities can be customized where they are defined and we don't have to use |
using the forum is a joke as they are down more than they are working, and i've yet to see a dev on discord to discuss with which is why i addressed it here. If packages aren't to be supported then the ability to use them needs to be removed entirely from documentation and all as this just divides the community. Power users like packages, why break this? I will not ever use the editors as they have proven to be garbage thus far and i should be able to continue to define my configs as i have been not be forced to put something in one place because someone said this doesn't make sense when it makes perfect sense. |
And how have you helped to fix that? Don't complain, help out.
And how have you helped to fix that? Don't complain, help out.
And how have you helped to fix that? Don't complain, help out. |
Description:
Google Assistant was using
customize
to store entity config. This is not allowed and is being addressed in this PR.This PR moves the entity config to be defined as part of the config for either
google_assistant
orcloud
.CC @philk
Related issue (if applicable): #10732 (Alexa done in #11461)
Breaking Change
Google Assistant is no longer configured via
customize
but instead has its configuration under thegoogle_assistant
entry in yourconfiguration.yaml
. The attributes will no longer have to be prefixed withgoogle_assistant_
either.Before:
After:
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml
(if applicable):Checklist:
If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass