Skip to content

Implement partial success response for set_characteristics#316

Merged
ikalchev merged 1 commit intoikalchev:devfrom
bdraco:set_chars_some_fail
Feb 27, 2021
Merged

Implement partial success response for set_characteristics#316
ikalchev merged 1 commit intoikalchev:devfrom
bdraco:set_chars_some_fail

Conversation

@bdraco
Copy link
Copy Markdown
Contributor

@bdraco bdraco commented Feb 14, 2021

Our current behavior is to send a HTTPStatus.BAD_REQUEST response when one of the setters generates an exception. This causes the controller to drop the connection which can lead to the looping unavailable state for all homekit devices.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 14, 2021

Codecov Report

Merging #316 (fd23c68) into dev (ec0a350) will increase coverage by 0.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #316      +/-   ##
==========================================
+ Coverage   82.76%   83.21%   +0.45%     
==========================================
  Files          19       19              
  Lines        1833     1859      +26     
  Branches      209      214       +5     
==========================================
+ Hits         1517     1547      +30     
+ Misses        248      245       -3     
+ Partials       68       67       -1     
Impacted Files Coverage Δ
pyhap/accessory_driver.py 82.12% <100.00%> (+1.79%) ⬆️
pyhap/hap_handler.py 75.20% <100.00%> (+0.88%) ⬆️

@bdraco bdraco marked this pull request as ready for review February 14, 2021 20:44
@bdraco
Copy link
Copy Markdown
Contributor Author

bdraco commented Feb 23, 2021

@ikalchev Any chance you can do a release with this so I can bump Home Assistant before tomorrow's beta?

@bdraco
Copy link
Copy Markdown
Contributor Author

bdraco commented Feb 27, 2021

@ikalchev sorry to ping again, hoping you have a moment now that we hit the weekend. Thanks!

@ikalchev ikalchev merged commit 3524c6e into ikalchev:dev Feb 27, 2021
@ikalchev
Copy link
Copy Markdown
Owner

Is tomorrow going to be too late?

@bdraco
Copy link
Copy Markdown
Contributor Author

bdraco commented Feb 27, 2021

Is tomorrow going to be too late?

Tomorrow is perfect. Thanks!

@ikalchev
Copy link
Copy Markdown
Owner

Hey I just released but some CI tests failed. As I am already out the door for a few hours, do you mind checking if everything is alright?

@bdraco
Copy link
Copy Markdown
Contributor Author

bdraco commented Feb 28, 2021

Just getting awake for the day (6am here). Will check it after coffee

@bdraco
Copy link
Copy Markdown
Contributor Author

bdraco commented Feb 28, 2021

Looks like it was just github being flakey and not being able to find the ref. It might have hit a backend server that was a second or so behind.

Error: fatal: couldn't find remote ref refs/pull/317/merge
Warning: Git fetch failed with exit code 128, back off 7.457 seconds before retry.
git -c http.extraheader="AUTHORIZATION: basic ***" fetch --tags --prune --progress --no-recurse-submodules origin +refs/heads/*:refs/remotes/origin/* +refs/pull/317/merge:refs/remotes/pull/317/merge
Error: fatal: couldn't find remote ref refs/pull/317/merge
Warning: Git fetch failed with exit code 128, back off 1.051 seconds before retry.
git -c http.extraheader="AUTHORIZATION: basic ***" fetch --tags --prune --progress --no-recurse-submodules origin +refs/heads/*:refs/remotes/origin/* +refs/pull/317/merge:refs/remotes/pull/317/merge

@bdraco
Copy link
Copy Markdown
Contributor Author

bdraco commented Feb 28, 2021

Tested and looks good. Also looks like the merge commit passed the ci so should be good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants