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

Google Assistant request sync service #10165

Merged
merged 7 commits into from
Nov 13, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion homeassistant/components/google_assistant/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@
from typing import Dict, Any # NOQA

from homeassistant.helpers import config_validation as cv
from homeassistant.helpers.aiohttp_client import async_get_clientsession
from homeassistant.loader import bind_hass

Choose a reason for hiding this comment

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

'homeassistant.loader.bind_hass' imported but unused


from .const import (
DOMAIN, CONF_PROJECT_ID, CONF_CLIENT_ID, CONF_ACCESS_TOKEN,
CONF_EXPOSE_BY_DEFAULT, CONF_EXPOSED_DOMAINS
CONF_EXPOSE_BY_DEFAULT, CONF_EXPOSED_DOMAINS,

Choose a reason for hiding this comment

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

trailing whitespace

CONF_AGENT_USER_ID, CONF_HOMEGRAPH_API_KEY,

Choose a reason for hiding this comment

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

trailing whitespace

SERVICE_REQUEST_SYNC, REQUEST_SYNC_BASE_URL
)
from .auth import GoogleAssistantAuthView
from .http import GoogleAssistantView
Expand All @@ -36,10 +40,15 @@
vol.Required(CONF_ACCESS_TOKEN): cv.string,
vol.Optional(CONF_EXPOSE_BY_DEFAULT): cv.boolean,
vol.Optional(CONF_EXPOSED_DOMAINS): cv.ensure_list,
vol.Required(CONF_AGENT_USER_ID): cv.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this optional and add a default for agent ID. Unless I misunderstood the docs there's no need for a unique agent ID since the integration maps 1:1 with an account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @philk, I was a bit unsure about this.

If HA were to ever become an official Google Assistant app, then the userId will have to be unique across all "cloud" users. Asking users to input their email address here may provide a bit of leeway in that regard.

Let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that's kind of why I think it should just be a default for now. If/when that happens it could be made required or the default modified based on some part of the cloud auth system.

vol.Required(CONF_HOMEGRAPH_API_KEY): cv.string
}
},
extra=vol.ALLOW_EXTRA)

def request_sync(hass):

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

"""Request sync."""
hass.services.call(DOMAIN, SERVICE_REQUEST_SYNC)

@asyncio.coroutine
def async_setup(hass: HomeAssistant, yaml_config: Dict[str, Any]):
Expand All @@ -49,4 +58,24 @@ def async_setup(hass: HomeAssistant, yaml_config: Dict[str, Any]):
hass.http.register_view(GoogleAssistantAuthView(hass, config))
hass.http.register_view(GoogleAssistantView(hass, config))

@asyncio.coroutine
def request_sync_service_handler(call):
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to move this out of the async_setup function 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @philk - confused by this comment. I have followed examples from automation, light inits. They all seem to define the service handler inside async_setup

Copy link
Contributor

Choose a reason for hiding this comment

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

Stylistically it looks weird to me but I looked through a few other components and that just seems to be the way to do it for hass/in python so nevermind, it works and matches so LGTM

"""Handle request sync service calls."""
#Code to execute request sync goes here

Choose a reason for hiding this comment

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

block comment should start with '# '

websession = async_get_clientsession(hass)

Choose a reason for hiding this comment

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

local variable 'websession' is assigned to but never used
trailing whitespace

try:
agent_user_id = config.get(CONF_AGENT_USER_ID)
with async_timeout.timeout(5, loop=hass.loop):

Choose a reason for hiding this comment

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

undefined name 'async_timeout'

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to import async_timeout (I might also make the timeout slightly higher but /shrug)

req = yield from session.post(

Choose a reason for hiding this comment

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

local variable 'req' is assigned to but never used
undefined name 'session'

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to do something with the request. I recommend raise_for_status() and separately catching and handling aiohttp.ClientResponseError and log the error mine looks like this

        except aiohttp.ClientResponseError:
            body = yield from res.read()
            _LOGGER.error(
                'sync request failed: %d %s', res.status, body)
            return

REQUEST_SYNC_BASE_URL+CONF_HOMEGRAPH_API_KEY,

Choose a reason for hiding this comment

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

trailing whitespace

Copy link
Contributor

Choose a reason for hiding this comment

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

The API key being part of params is cleaner: params={'key': self.api_key} as another kwarg.

json={'agent_user_id': agent_user_id})
_LOGGER.info("Submitted request_sync request to Google")
except (asyncio.TimeoutError, aiohttp.ClientError):

Choose a reason for hiding this comment

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

undefined name 'aiohttp'

Copy link
Contributor

Choose a reason for hiding this comment

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

^ import aiohttp

_LOGGER.error("Could not contact Google for request_sync")
return None
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent return

Copy link
Member

Choose a reason for hiding this comment

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

Think you can just remove all of them?


Choose a reason for hiding this comment

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

blank line contains whitespace

hass.services.async_register(
DOMAIN, SERVICE_REQUEST_SYNC, request_sync_service_handler,
descriptions.get(SERVICE_REQUEST_SYNC))

Choose a reason for hiding this comment

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

undefined name 'descriptions'

Copy link
Contributor

Choose a reason for hiding this comment

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

^ going to need a services.yaml and initialize the description (look for another component that has descriptions.get like that and you'll find a pretty standard pattern)


return True
6 changes: 6 additions & 0 deletions homeassistant/components/google_assistant/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
CONF_ACCESS_TOKEN = 'access_token'
CONF_CLIENT_ID = 'client_id'
CONF_ALIASES = 'aliases'
CONF_AGENT_USER_ID = 'agent_user_id'
CONF_HOMEGRAPH_API_KEY = 'homegraph_api_key'
Copy link
Contributor

Choose a reason for hiding this comment

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

The API key isn't specific to homegraph, but to the project as a whole. So maybe rename to just api_key.


DEFAULT_EXPOSE_BY_DEFAULT = True
DEFAULT_EXPOSED_DOMAINS = [
Expand All @@ -35,3 +37,7 @@
TYPE_LIGHT = PREFIX_TYPES + 'LIGHT'
TYPE_SWITCH = PREFIX_TYPES + 'SWITCH'
TYPE_SCENE = PREFIX_TYPES + 'SCENE'

SERVICE_REQUEST_SYNC = 'request_sync'
HOMEGRAPH_URL = 'https://homegraph.googleapis.com/'
REQUEST_SYNC_BASE_URL = HOMEGRAPH_URL + 'v1/devices:requestSync?key='
9 changes: 7 additions & 2 deletions homeassistant/components/google_assistant/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
DEFAULT_EXPOSED_DOMAINS,
CONF_EXPOSE_BY_DEFAULT,
CONF_EXPOSED_DOMAINS,
ATTR_GOOGLE_ASSISTANT)
ATTR_GOOGLE_ASSISTANT,
CONF_AGENT_USER_ID
)
from .smart_home import entity_to_device, query_device, determine_service

_LOGGER = logging.getLogger(__name__)
Expand All @@ -48,6 +50,7 @@ def __init__(self, hass: HomeAssistant, cfg: Dict[str, Any]) -> None:
DEFAULT_EXPOSE_BY_DEFAULT)
self.exposed_domains = cfg.get(CONF_EXPOSED_DOMAINS,
DEFAULT_EXPOSED_DOMAINS)
self.agent_user_id = cfg.get(CONF_AGENT_USER_ID)

def is_entity_exposed(self, entity) -> bool:
"""Determine if an entity should be exposed to Google Assistant."""
Expand Down Expand Up @@ -85,7 +88,9 @@ def handle_sync(self, hass: HomeAssistant, request_id: str):
devices.append(device)

return self.json(
make_actions_response(request_id, {'devices': devices}))
make_actions_response(request_id,

Choose a reason for hiding this comment

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

trailing whitespace

{'agentUserId': self.agent_user_id,

Choose a reason for hiding this comment

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

indentation contains tabs
indentation contains mixed spaces and tabs
continuation line under-indented for visual indent
trailing whitespace

'devices': devices}))

Choose a reason for hiding this comment

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

indentation contains tabs
indentation contains mixed spaces and tabs
continuation line under-indented for visual indent


@asyncio.coroutine
def handle_query(self,
Expand Down