Squeezebox: Fix Allow server device details to merge with players with the same MAC #133517
Conversation
|
Hey there @rajlaud, @peteS-UK, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Something whacky with services vs devices. I see all of my software players as services. The only device is my one hardware player...
|
|
@pssc this seems to be a logical approach. It will allow us to associate entities with either the server or the player as appropriate. For example, the alarms show up as associated with the player in my alarms PR (#127933). 
@peteS-UK maybe you need to delete add re-add your LMS server? I'm seeing a software player (squeezelite) properly show up as a device and the server as a service. 
|
Hi @rajlaud . No, it was a problem with an earlier version which Phil has already fixed. |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
@pssc tests are failing, can you take a look? Please mark the PR "Ready for review" when you've addressed the failing tests. |
|
Thanks @emontnemery fixed tests |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where a Squeezebox server and its player were being merged into a single device by preserving the server’s MAC details. The changes include renaming entity IDs in tests, updating snapshot expectations, and altering the device merging logic in the media player platform.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/components/squeezebox/test_switch.py | Updated switch entity IDs from "test_player_alarm" to "none_alarm". |
| tests/components/squeezebox/test_media_player.py | Added a test to ensure proper device registry merging for servers. |
| tests/components/squeezebox/test_button.py | Updated button entity IDs accordingly. |
| tests/components/squeezebox/snapshots/test_switch.ambr | Updated snapshot entries to reflect new naming. |
| tests/components/squeezebox/snapshots/test_media_player.ambr | Adjusted snapshot expectations on device entries. |
| tests/components/squeezebox/conftest.py | Modified TEST_MAC and adjusted server mock details. |
| homeassistant/components/squeezebox/media_player.py | Revised the _player_discovered function to merge server and player info. |
| homeassistant/components/squeezebox/entity.py | Removed direct device info attributes to favor merged details. |
| homeassistant/components/squeezebox/const.py | Updated constant names to reflect server details. |
| homeassistant/components/squeezebox/init.py | Updated server device creation with new manufacturer/model details. |
Co-authored-by: Copilot <[email protected]>
|
@peteS-UK @pssc I think this PR looks good. However, there's an unresolved conversation above: #133517 (comment) Can you comment on if that's still relevant? |
Proposed change
Fixes a problem where a server and player would be merged into a single device / service
This is a different tack on #128989 that allows us to keep the server mac.
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: