-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
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
Patch service validation in Aussie Broadband #99077
Conversation
Hey there @nickw444, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
I'm releasing 0.1.2 to bump the pydantic version to a 2.0 minimum, which should sort the tests. |
@yaleman, I'm no expert on this field, but there was a discussion about pydantic 2. A lot of libraries are solving it like https://github.com/AngellusMortis/pyunifiprotect/pull/297 did. Maybe it can help? |
I'm not rewriting my module to go back to the old version, sorry. If hass supports specifying a branch/tag as a requirement I'll backport the fix, or you could patch the old version after import by overriding the affected class method (it's just a check for "known-valid" service types.) |
I discussed this with someone else and the problem is that currently HA only runs Pydantic v1. The bad news is that we can't add libraries that run only with Pydantic v2. As it will break the libraries still running with v1. You can see integration owners have started updating libraries with the v1 shims (as you can see in the PR I linked). We didn't have much pressure to upgrade. In the coming time I will try to migrate the rest of the libraries so we can safely bump pydantic to v2. |
Fixed without having to use pydantic 2.x - just patching the "broken" code in the 0.0.15 release. |
@yaleman would you consider backporting this fix to a 0.0.x version of the library? |
I'd rather not spend any more time chasing fixes specific to Home Assistant especially when there's a fixed version already available, and this PR fixes your issue. |
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.
Please also add a test that fails if we start using pydantic 2+ in Home Assistant.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
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.
Thanks!
* Bump pyAussieBB * rolling back to previous version * patching the pydantic 2.x issue in aussie_broadband integration * adding test for validate_service_type * adding test for validate_service_type * fixing tests, again * adding additional test * doing fixes for live tests * Implement Feedback * Add test to detect pydantic2 * Update test_init.py * Update docstring --------- Co-authored-by: James Hodgkinson <james@terminaloutcomes.com>
Proposed change
The library pyAussieBB now requries Pydantic v2, which Home Assistant does not support, so we are not longer able to update the library to address issues or add service types. #99218
So to fix #95665, an override has been implemented in this PR.
Type of change
Additional information
yaleman/pyaussiebb@v0.0.15...v0.1.1
Checklist
black --fast 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
..coveragerc
.To help with the load of incoming pull requests: