Convert Vallox integration to config flow #62780

slovdahl · 2021-12-25T20:45:58Z

Breaking change

The Vallox integration migrated to configuration via the UI. Configuring Vallox via YAML configuration has been deprecated and will be removed in a future Home Assistant release.

Your existing YAML configuration is automatically imported on upgrade to this release; and thus can be safely removed from your YAML configuration after upgrading.

Proposed change

Update the Vallox integration to support UI configuration, and make the initialization of the integration more idiomatic. See #62308 (comment) and #61708 for more context.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue: Vallox integration broken with 2021.12 (even with fix after 2021.12.3) #61708
Link to documentation pull request: Update Vallox documentation for config flow home-assistant.io#20861

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2021-12-25T20:46:08Z

Hey there @andre-richter, mind taking a look at this pull request as it has been labeled with an integration (vallox) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

bdraco

Thanks. 👍

Please add test coverage for the config flow. We need 100% for these.

homeassistant/components/vallox/__init__.py

andre-richter · 2021-12-25T23:36:44Z

Awesome. Thanks a lot for your effort 👍

slovdahl · 2021-12-26T07:14:00Z

Thanks. 👍

Please add test coverage for the config flow. We need 100% for these.

Good point, added some more tests and improved the quality of the existing tests a bit too. How can I check the test coverage locally? 🤔 I ran tox -e cov -- tests/components/vallox locally before I opened the PR but that was all green. The reports available from CI were very useful.

bdraco · 2021-12-26T07:20:32Z

pytest --cov=homeassistant/components/vallox/ --cov-report term-missing -- tests/components/vallox/

Inside the venv

Related to home-assistant/core#62780.

slovdahl · 2021-12-26T09:34:42Z

Updated docs: home-assistant/home-assistant.io#20861

slovdahl · 2021-12-26T11:49:17Z

Please add test coverage for the config flow. We need 100% for these.

This has now been fixed.

slovdahl · 2021-12-26T11:51:37Z

One thing I really don't know is if it's possible to support unloading the integration with async_unload_entry. I did add it from the start, but realized there might be things that need to be cleaned up (closing the WebSocket connection maybe?). If someone is able to point me in the right direction with regard to this I could probably fix it at the same time.

andre-richter · 2021-12-26T13:24:41Z

One thing I really don't know is if it's possible to support unloading the integration with async_unload_entry. I did add it from the start, but realized there might be things that need to be cleaned up (closing the WebSocket connection maybe?). If someone is able to point me in the right direction with regard to this I could probably fix it at the same time.

I peeked into the vallox websocket API. Although I don’t know anything about websockets, it seems a connection is created (and closed again) for each request, and not once on class instantiation and kept open. So maybe no cleanup needed.

https://github.com/yozik04/vallox_websocket_api/blob/5c45be59e46bcd9cf9a14c92b4b1f48d86f38191/vallox_websocket_api/client.py#L195-L204

homeassistant/components/vallox/config_flow.py

slovdahl · 2021-12-27T18:47:15Z

Please run python3 -m script.hassfest

Passes locally:

root ➜ /workspaces/core (vallox-config-flow) $ python3 -m script.hassfest
Validating codeowners... done in 0.02s
Validating config_flow... done in 0.03s
Validating dependencies... done in 9.33s
Validating dhcp... done in 0.00s
Validating json... done in 0.00s
Validating manifest... done in 0.07s
Validating mqtt... done in 0.00s
Validating requirements... done in 0.02s
Validating services... done in 3.21s
Validating ssdp... done in 0.00s
Validating translations... done in 1.47s
Validating usb... done in 0.00s
Validating zeroconf... done in 0.00s
Validating coverage... done in 0.06s
Validating mypy_config... done in 0.02s

Integrations: 1020
Invalid integrations: 0

bdraco · 2021-12-27T18:53:36Z

I checked the CI, and it looks like the problem was github having issues at the time and everything is fine with hassfest. I restarted the CI.

homeassistant/components/vallox/config_flow.py

bdraco

Looks good 👍

Would you please retest yaml imports before I merge this?

homeassistant/components/vallox/__init__.py

andre-richter · 2021-12-27T23:01:35Z

Would you please retest yaml imports before I merge this?

I just tested this, and noticed no issues 👍. Config entry created from import was there, and you get the warning/hint to remove the yaml entry.

slovdahl · 2021-12-28T07:11:32Z

Would you please retest yaml imports before I merge this?

Tested everything once more, seems to work in line with expectations.

homeassistant/components/vallox/sensor.py

MartinHjelmare

Please address the comments in a new PR. Thanks!

MartinHjelmare · 2021-12-28T21:14:04Z

homeassistant/components/vallox/config_flow.py

+        try:
+            await validate_host(self.hass, host)
+        except InvalidHost:
+            _LOGGER.exception("An invalid host is configured for Vallox")


We normally only log the stack trace for unknown exceptions.

Thanks, fixed this and all other comments in #63032.

MartinHjelmare · 2021-12-28T21:19:38Z

homeassistant/components/vallox/strings.json

+    "step": {
+      "user": {
+        "title": "Vallox",
+        "description": "Set up the Vallox integration. If you have problems with configuration go to https://www.home-assistant.io/integrations/vallox.",


Links are best to put in a description placeholder to avoid translation of the link.

MartinHjelmare · 2021-12-28T21:20:57Z

homeassistant/components/vallox/strings.json

+        "description": "Set up the Vallox integration. If you have problems with configuration go to https://www.home-assistant.io/integrations/vallox.",
+        "data": {
+          "host": "[%key:common::config_flow::data::host%]",
+          "name": "[%key:common::config_flow::data::name%]"


There's no name item in the user step form.

MartinHjelmare · 2021-12-28T21:21:44Z

homeassistant/components/vallox/strings.json

+    "error": {
+      "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
+      "invalid_host": "[%key:common::config_flow::error::invalid_host%]"
+    }


We're missing a string for unknown error.

MartinHjelmare · 2021-12-28T21:22:23Z

homeassistant/components/vallox/strings.json

+    },
+    "abort": {
+      "already_configured": "[%key:common::config_flow::abort::already_configured_service%]"
+    },


We're missing many abort reasons.

Looks like invalid_host, cannot_connect, unknown are missing here

MartinHjelmare · 2021-12-28T21:27:16Z

tests/components/vallox/test_config_flow.py

+    assert result["reason"] == "already_configured"
+
+
+async def test_import_cannot_connect_OSError(hass: HomeAssistant) -> None:


Please use lowercase snake_case function names.

👍

I thought about it too, but because no linter or pre-commit check complained I just assumed it was fine. Could/should it be enforced somehow? 🤔

Convert Vallox integration to config flow

c84384d

homeassistant added the cla-signed label Dec 25, 2021

probot-home-assistant bot added breaking-change code-quality has-tests integration: vallox labels Dec 25, 2021

slovdahl mentioned this pull request Dec 25, 2021

Vallox integration broken with 2021.12 (even with fix after 2021.12.3) #61708

Closed

bdraco requested changes Dec 25, 2021

View reviewed changes

andre-richter reviewed Dec 25, 2021

View reviewed changes

homeassistant/components/vallox/__init__.py Show resolved Hide resolved

slovdahl added 2 commits December 26, 2021 07:10

Improve test coverage

93ae4ee

Remove unused constant

ac75e7e

slovdahl added a commit to slovdahl/home-assistant.io that referenced this pull request Dec 26, 2021

Update Vallox documentation for config flow

17e4b3c

Related to home-assistant/core#62780.

slovdahl mentioned this pull request Dec 26, 2021

Update Vallox documentation for config flow home-assistant/home-assistant.io#20861

Merged

8 tasks

slovdahl added 2 commits December 26, 2021 09:39

Fix handling of existing entry on import

c8db3ed

Add another test

7f69fea

bdraco reviewed Dec 26, 2021

View reviewed changes

homeassistant/components/vallox/config_flow.py Show resolved Hide resolved

bdraco reviewed Dec 26, 2021

View reviewed changes