Fix snapcast uuid to be more unique#14925
Conversation
homeassistant
added
cla-signed
platform: media_player.snapcast
small-pr
ghost
added
There was a problem hiding this comment.
line too long (99 > 79 characters)
There was a problem hiding this comment.
line too long (99 > 79 characters)
There was a problem hiding this comment.
line too long (96 > 79 characters)
There was a problem hiding this comment.
line too long (96 > 79 characters)
There was a problem hiding this comment.
Changing the name will change the entity_id, so that will make this a breaking change.
With the unique id set, the entity_id doesn't need to be changed. Both name and entity_id can be customized by the user.
There was a problem hiding this comment.
Hmm, I see. Reason for changing it was the same name twice in the tab.
So should I keep the previous name and make renaming inside the registry file?
There was a problem hiding this comment.
That's my suggestion. I'm guessing not all users will need to change this if they don't have multiple hosts. So it's better to not make a breaking change and still allow impacted users to set their names and IDs as they want.
Current uuid is ok when using only 1 snapserver New uuid is needed when using multiple snapserver Because the client can connect to more snapservers and then uuid based on client MAC is not enough
|
I must changed the way of getting host, port because travis does not like the access of "private" members |
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Thanks!
Sidenote: Please don't squash commits after review has started. It makes it harder to follow the changes. We will squash via github when merging.
|
@MartinHjelmare Isn't this a breaking change? When the unique ID changes, the entity registry will generate a new entry for it and thus a new entity ID. (side note: changing the name of an entity that implements unique_id is AFAIK not a breaking change, as the entity registry keeps entity IDs with the same unique ID static) |
|
That's true. But since the unique_id was added five days ago it hasn't been released yet, so it's not a breaking change and my statements above was based on that. |
5 participants
Current uuid is ok when using only 1 snapserver
New uuid is needed when using multiple snapserver
Because the client can connect to more snapservers and
then uuid based on client MAC is not enough
Description:
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: