Split blocking of fulfillment from state reporting in Google Assistant #126539
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @coling,
When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).
The commits that are missing a linked GitHub account are the following:
5d2e1ecb5797eba48d2dfc9adad9efaef0202285- This commit has something that looks like an email address (dev@colin.guthr.ie). Maybe try linking that to GitHub?.
Unfortunately, we are unable to accept this pull request until this situation is corrected.
Here are your options:
-
If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
-
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
(substituting "Author Name" and "
git commit --amend --author="Author Name <email@address.com>"email@address.com" for your actual information) to set the authorship information. - If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
- You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
- You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.
- If you only made a single commit you should be able to run
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.
Thanks, I look forward to checking this PR again soon! ❤️
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @home-assistant/cloud, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
MartinHjelmare
changed the title
coling
force-pushed
the
google-assistant-sync-fulfillment
branch
from
5d2e1ec to
b3449b4
Compare
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
The Google Home Graph API says fulfillment should be sync, but in HA it's async if state reporting is turned on. This can cause some UI issues in Google Home App. To work around this, add a separate control over whether fulfillment is async or blocking but default to the current behaviour. See issue home-assistant#125793
coling
force-pushed
the
google-assistant-sync-fulfillment
branch
from
b3449b4 to
cc89d03
Compare
|
Is there any reason not to merge this? Is there any feedback so I can incorporate it? Thanks for all your awesome work! |
|
Okay so I only recently did some tinkering to google_assistant so I know a bit about the context, but not all. So if you could help me out a bit that would be amazing :) So Google wants to get stuff executed sync, while we do it async? Why would one have report state turned off (I think that is a question as well) and have async fulfillment turned on? |
Well, I'm a green as they come both with development for HA and with GA, so I wouldn't take what I say as gospel by any stretch. I wrote my patch on the basis of a "don't change any current behaviour" approach (don't break it!!) but, personally, to me the docs definitely state that the response should be the new state. I don't know if there are any additional response timing constraints that need to be factored in. This doesn't actually specify that the operation has to be sync, just that the response has to be the new state. i.e. we could just respond with what the new state should be - obviously this could fail, but that should then be updated via state reporting anyway. I'm not sure if this is a healthy approach or not! I'm also not sure how easy it would be to simulate the expected new state for the response (it might be quite tricky) and thus executing sync might be the most sensible way. Anyway, that's my rationale for the patch as it stands but happy to take feedback and adjust accordingly. |
It does specify an order of operations with the new state being returned only after transmitting requests to the various devices (e.g., sending an Zigbee genOnOff cluster command), which suggests that the EXECUTE as a whole should be sync. As such, I'm tempted to suggest that we simplify this to always be sync without any configuration. |
The Google Home Graph API says fulfillment should be sync, but in HA it's async if state reporting is turned on. This can cause some UI issues in Google Home App.
To work around this, add a separate control over whether fulfillment is async or blocking but default to the current behaviour.
See issue #125793
Proposed change
This allows fine grained control over whether the processing of EXECUTE intents is synchronous or asynchronous. It is currently tied to whether the report_state configuration is turned on or not but this seems incorrect. The upstream docs (as quoted in #125793) suggest that fulfilment should be synchronous, however this pull does not change the current behaviour - just affords more control.
Arguably, it may make more sense to just disable asynchronous fulfillment altogether if there is no compelling reason not to.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
Not yet, but I will add this based on feedback
To help with the load of incoming pull requests: