Updated to new HA standards, and added ssl support#21
Updated to new HA standards, and added ssl support#21arnonm wants to merge 4 commits intoshaiu:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for custom ports and SSL configurations to the Technicolor integration, including a new options flow for post-setup adjustments. It also implements entry unloading and refines logging levels. Review feedback identifies a critical bug where ConfigEntryNotReady is returned instead of raised, a potential KeyError during unloading, and the use of protected members from an external library, which poses a maintenance risk.
| try: | ||
| await self.loop.run_in_executor(None, self._api.authenticate) | ||
| except Exception as e: | ||
| _LOGGER.exception("Failed to connect to Technicolor", e) | ||
| except Exception: | ||
| _LOGGER.exception("Failed to connect to Technicolor") | ||
| return ConfigEntryNotReady |
There was a problem hiding this comment.
The setup method returns the ConfigEntryNotReady exception class instead of raising it. In Home Assistant, ConfigEntryNotReady should be raised during the setup process to signal that the integration should retry later (e.g., if the device is temporarily offline). Returning it causes the setup to proceed as if it succeeded, which will lead to errors when the integration attempts to use the unauthenticated API.
| try: | |
| await self.loop.run_in_executor(None, self._api.authenticate) | |
| except Exception as e: | |
| _LOGGER.exception("Failed to connect to Technicolor", e) | |
| except Exception: | |
| _LOGGER.exception("Failed to connect to Technicolor") | |
| return ConfigEntryNotReady | |
| try: | |
| await self.loop.run_in_executor(None, self._api.authenticate) | |
| except Exception: | |
| _LOGGER.exception("Failed to connect to Technicolor") | |
| raise ConfigEntryNotReady |
| router = hass.data[DOMAIN].pop(entry.entry_id)[DOMAIN] | ||
| router.async_unload() |
There was a problem hiding this comment.
Directly popping from hass.data[DOMAIN] using the entry_id can raise a KeyError if the entry was not fully initialized or if it has already been removed. It is safer to handle the case where the entry ID is missing.
| router = hass.data[DOMAIN].pop(entry.entry_id)[DOMAIN] | |
| router.async_unload() | |
| router = hass.data[DOMAIN].pop(entry.entry_id, {}).get(DOMAIN) | |
| if router: | |
| router.async_unload() |
| self._api._uri = f"{scheme}://{self._host}:{self._port}" | ||
| if self._use_ssl and not self._verify_ssl: | ||
| self._api._br.session.verify = False |
There was a problem hiding this comment.
Accessing and modifying protected members of an external library (like _uri and _br in TechnicolorGateway) is risky and can lead to breakage if the library's internal implementation changes. It is better to use public APIs if available, or consider contributing the necessary SSL support to the upstream library.
No description provided.