-
-
Notifications
You must be signed in to change notification settings - Fork 35.9k
Add config flow for Vivotek integration #154801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Add config flow for Vivotek integration #154801
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be made to switch-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so because we need all of these not just one of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need so many safe-guards? Aren't there better solutions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have this assert? What happens when this is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
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: