Add config flow for Vivotek integration#154801
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds config flow support to the existing Vivotek integration, enabling users to configure Vivotek IP cameras through the Home Assistant UI instead of only through YAML configuration. The implementation includes both initial setup and reconfiguration capabilities.
Key changes:
- Adds config flow implementation with user setup and reconfiguration steps
- Updates the integration to support both YAML and config entry setup methods
- Extracts constants to a separate module for better organization
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/vivotek/config_flow.py |
New config flow implementation with user and reconfigure steps |
homeassistant/components/vivotek/const.py |
New constants file extracting domain-specific configuration keys |
homeassistant/components/vivotek/strings.json |
New localization strings for config flow UI |
homeassistant/components/vivotek/manifest.json |
Enables config flow support in manifest |
homeassistant/components/vivotek/camera.py |
Updates camera platform to support config entries and refactors existing code |
homeassistant/components/vivotek/__init__.py |
New entry point for config entry setup and unloading |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
erwindouna
left a comment
There was a problem hiding this comment.
Making good progress, @HarlemSquirrel! I've left some points. :)
| if "ip_address" in import_data: | ||
| user_input[CONF_IP_ADDRESS] = import_data["ip_address"] | ||
| if "name" in import_data: | ||
| user_input[CONF_NAME] = import_data["name"] | ||
| if "username" in import_data: | ||
| user_input[CONF_USERNAME] = import_data["username"] | ||
| if "password" in import_data: | ||
| user_input[CONF_PASSWORD] = import_data["password"] | ||
| if "authentication" in import_data: | ||
| user_input[CONF_AUTHENTICATION] = import_data["authentication"] | ||
| if "ssl" in import_data: | ||
| user_input[CONF_SSL] = import_data["ssl"] | ||
| if "verify_ssl" in import_data: | ||
| user_input[CONF_VERIFY_SSL] = import_data["verify_ssl"] | ||
| if "framerate" in import_data: | ||
| user_input[CONF_FRAMERATE] = import_data["framerate"] | ||
| if "security_level" in import_data: | ||
| user_input[CONF_SECURITY_LEVEL] = import_data["security_level"] | ||
| if "stream_path" in import_data: | ||
| user_input[CONF_STREAM_PATH] = import_data["stream_path"] |
There was a problem hiding this comment.
Can this be made to switch-case?
There was a problem hiding this comment.
I don't think so because we need all of these not just one of them
There was a problem hiding this comment.
Why do we need so many safe-guards? Aren't there better solutions?
There was a problem hiding this comment.
I think it's good to note that the validation of the existing YAML still stands, so we already know what we could expect. So yes, I think we can improve this
| sec_lvl=data[CONF_SECURITY_LEVEL], # type: ignore[literal-required] | ||
| ) | ||
| mac = await hass.async_add_executor_job(cam_client.get_mac) | ||
| assert len(mac) > 0 |
There was a problem hiding this comment.
Why do we have this assert? What happens when this is the case?
There was a problem hiding this comment.
Was using it as a way to fail faster but can remove it
| "doc_url": "https://www.home-assistant.io/integrations/vivotek/" | ||
| } | ||
|
|
||
| CONF_SCHEMA = vol.Schema( |
There was a problem hiding this comment.
Also note, we have support for section() now. Check telegram_bot for an example. It might help splitting easy stuff from complex stuff.
I would also consider that everything that is not part of the connection details (like host, auth) can be an options flow as it would just be configuration.
| if "ip_address" in import_data: | ||
| user_input[CONF_IP_ADDRESS] = import_data["ip_address"] | ||
| if "name" in import_data: | ||
| user_input[CONF_NAME] = import_data["name"] | ||
| if "username" in import_data: | ||
| user_input[CONF_USERNAME] = import_data["username"] | ||
| if "password" in import_data: | ||
| user_input[CONF_PASSWORD] = import_data["password"] | ||
| if "authentication" in import_data: | ||
| user_input[CONF_AUTHENTICATION] = import_data["authentication"] | ||
| if "ssl" in import_data: | ||
| user_input[CONF_SSL] = import_data["ssl"] | ||
| if "verify_ssl" in import_data: | ||
| user_input[CONF_VERIFY_SSL] = import_data["verify_ssl"] | ||
| if "framerate" in import_data: | ||
| user_input[CONF_FRAMERATE] = import_data["framerate"] | ||
| if "security_level" in import_data: | ||
| user_input[CONF_SECURITY_LEVEL] = import_data["security_level"] | ||
| if "stream_path" in import_data: | ||
| user_input[CONF_STREAM_PATH] = import_data["stream_path"] |
There was a problem hiding this comment.
I think it's good to note that the validation of the existing YAML still stands, so we already know what we could expect. So yes, I think we can improve this
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
joostlek
left a comment
There was a problem hiding this comment.
So I pushed to the branch as i also didn't want to come back with another bunch of comments, instead this now is in a mergeable state after which we can start improving the integration :)
Proposed change
Add config flow for Vivotek integration
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: