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

Move Google Assistant entity config out of customize #11499

Merged
merged 3 commits into from
Jan 9, 2018

Conversation

balloob
Copy link
Member

@balloob balloob commented Jan 7, 2018

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 or cloud.

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 the google_assistant entry in your configuration.yaml. The attributes will no longer have to be prefixed with google_assistant_ either.

Old option New option
google_assistant expose
aliases aliases
google_assistant_name name
google_assistant_type type

Before:

homeassistant:
  customize:
    switch.kitchen:
      google_assistant: false
      google_assistant_name: nice lights
      google_assistant_type: light
      aliases:
        - roof lights

google_assistant:

After:

google_assistant:
  entity_config:
    switch.kitchen:
      expose: false
      alias: roof lights
      name: nice lights
      type: light

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:

  • The code change is tested and works locally.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Copy link
Contributor

@philk philk left a 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)
Copy link
Contributor

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Copy link
Member Author

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)

Copy link
Member Author

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,
Copy link
Contributor

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)

Copy link
Member Author

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.

@balloob
Copy link
Member Author

balloob commented Jan 9, 2018

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 google_assistant component.

Copy link
Contributor

@philk philk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@balloob balloob merged commit 8313225 into dev Jan 9, 2018
@balloob balloob deleted the google-assistant-config branch January 9, 2018 23:15
@balloob
Copy link
Member Author

balloob commented Jan 9, 2018

Will work on an update to the docs today.

@ChristopherLMiller
Copy link

ChristopherLMiller commented Jan 15, 2018

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.
https://github.com/moose51789/home-assistant-config/blob/master/packages/lights/modeling_desk.yaml

@MartinHjelmare
Copy link
Member

Please don't use closed PRs for discussion. Use the forum or discord.

@rofrantz
Copy link
Contributor

rofrantz commented Jan 15, 2018

Without trying to be a smartass or trying to offend anyone:

  1. Devs read pr's to see what will "hit" them; endusers use forums, after pr's are merged; @moose51789 is probably trying to trigger a warning before this hits us

  2. I know that "a decision has already been taken" and all main devs praise the fact that it will be easier to reload ha without restart (i know this because i also watch & read pr, NOT forums) but think also at the price we pay for this and I think that moose51789 has a point here;

  3. This is history repeating: the same (bad imho) thing happend also with zwave entity ids where we had 3-4 breaking changes and if i'm not mistaking there's now a 4th by the man itself (ballob). And again all those changes for easier custmization from ui config panel and harder and stupider automation configs for devs like the real case example that moose51789 gave

@arsaboo
Copy link
Contributor

arsaboo commented Jan 15, 2018

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 customize to scatter our configs.

@ChristopherLMiller
Copy link

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.

@home-assistant home-assistant locked and limited conversation to collaborators Jan 15, 2018
@balloob
Copy link
Member Author

balloob commented Jan 15, 2018

using the forum is a joke as they are down more than they are working

And how have you helped to fix that? Don't complain, help out.

I will not ever use the editors as they have proven to be garbage thus far

And how have you helped to fix that? Don't complain, help out.

i should be able to continue to define my configs as i have been

And how have you helped to fix that? Don't complain, help out.

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.

8 participants