Skip to content

Add websocket API for updating core config#24009

Merged
emontnemery merged 4 commits intohome-assistant:devfrom
emontnemery:core_update_websocket
May 21, 2019
Merged

Add websocket API for updating core config#24009
emontnemery merged 4 commits intohome-assistant:devfrom
emontnemery:core_update_websocket

Conversation

@emontnemery
Copy link
Copy Markdown
Contributor

Description:

Add new websocket API for updating core config

This is a follow-up to: #23872, #23922

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@ghost
Copy link
Copy Markdown

ghost commented May 20, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (config) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@emontnemery
Copy link
Copy Markdown
Contributor Author

Is the WS API documented, and if so should this be added to the docs?

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Ok to merge when tests updated. I have the UI ready in a branch 😎


assert msg['id'] == 5
assert msg['type'] == TYPE_RESULT
assert msg['success']
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please verify latitude got set, preferably test all other values too

Copy link
Copy Markdown
Contributor Author

@emontnemery emontnemery May 20, 2019

Choose a reason for hiding this comment

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

Fixed!

I had a review comment which I forgot to publish: should the new WS command config/core/update return:

  • The set value(s)
  • The updated core config

Or is it fine like it is?

data.pop('id')
data.pop('type')
await hass.config.update(**data)
connection.send_result(msg['id'])
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we want to return something here?

@emontnemery emontnemery merged commit fc58746 into home-assistant:dev May 21, 2019
@balloob balloob mentioned this pull request Jun 4, 2019
@emontnemery emontnemery deleted the core_update_websocket branch September 6, 2019 10:02
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Oct 12, 2019
* Add websocket API for updating core config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants