Skip to content
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

fix!: Services.yaml // New Identifier #287

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

Conversation

Kolbi
Copy link
Contributor

@Kolbi Kolbi commented Mar 12, 2024

Currently need some testing as I'm changing the identifier:
Instead of:
"identifiers": {(DOMAIN, self._instrument.vehicle_name)},
We now use:
"identifiers": {(DOMAIN, self._instrument.vehicle_vin)},

This makes the Services.yaml working smooth and a name can't be a unique id.

But side-effects has to be tested. I had two vehicles after the change but might be my settings related.

I thought maybe if we update the identifier but I couldn't find an easy solution for it.

But we now have the possibility to delete devices / vehicles from UI.

@cdnninja
Copy link
Collaborator

I will test this out when I get a chance. We may be able to also do a config migration on this. https://developers.home-assistant.io/docs/config_entries_config_flow_handler/#config-entry-migration

@cdnninja
Copy link
Collaborator

I haven't had a chance to look at this yet been a busy week.

@coreywillwhat
Copy link
Contributor

coreywillwhat commented Mar 26, 2024

I reviewed, tested, and merged into my fork. Absolutely wonderful having the Device drop down in the service calls. Thanks for doing this!

Note: As you mentioned, i also had a blank, duplicate device, where the "Room" was still attached. Seems its creating a new device and removing the details from the previous, other than the room. The new device did not have a room assigned.

I deleted the integration > restarted HA > readded the AudiConnect integration, and then only had 1 vehicle.

Otherwise this worked like a charm.

@Kolbi
Copy link
Contributor Author

Kolbi commented Mar 27, 2024

I reviewed, tested, and merged into my fork. Absolutely wonderful having the Device drop down in the service calls. Thanks for doing this!

Note: As you mentioned, i also had a blank, duplicate device, where the "Room" was still attached. Seems its creating a new device and removing the details from the previous, other than the room. The new device did not have a room assigned.

I deleted the integration > restarted HA > readded the AudiConnect integration, and then only had 1 vehicle.

Otherwise this worked like a charm.

I was trying to write an migration definition see "async def async_migrate_entry": https://github.com/Kolbi/audi_connect_ha/blob/b075dbd16cce736be4272f9487eeda99b9b27012/custom_components/audiconnect/__init__.py

but it doesn't work.... I even don't see in any entries from it in the logs....

@cdnninja
Copy link
Collaborator

I hope to test this soon. Sorry crazy busy with the toddler and baby!

For migration here is what I did on another project when had the same issue. https://github.com/Hyundai-Kia-Connect/kia_uvo/blob/894299ab58774c08f3a6f7b66fa24229f06799ef/custom_components/kia_uvo/__init__.py#L75-L123

Probably could be cleaner but it worked.

I assume this would be a breaking change? If so we can rename it so the release engine will make this a 2.x release and label is breaking.

@Kolbi
Copy link
Contributor Author

Kolbi commented Mar 28, 2024

I know this PR is not needed anymore but want to let it open until we finished the "async_migrate_entry" topic and implemented it in @coreywillwhat PR.

I still have no clue why it is not working, but currently i don't have any testing enviroment which makes testing a little bit more difficult. :)

@coreywillwhat
Copy link
Contributor

coreywillwhat commented Mar 28, 2024

I tried your code block @Kolbi . Having the same issue where it never initiates the change/no logs.

A little more digging shows the version is never set for the entry in the config flow upon initial integration setup, so when you're looking for if config_entry.version == 1: maybe it isn't meeting the criteria?

In the AudiConfigFlow I think we need to define the version similar to how @cdnninja did in his referenced hyundai/kia integration.

A good starting place might be to figure out what version it's been assigning, if anything, then account for that, and assign the new VERSION = 2 in the config_flow.py so we dont run into this in the future.

Edit: FWIW -- This time around I tried deleting the duplicate device and I haven't encountered any issues. the "new" device maintains the correct entity id's so existing references aren't impacted.

Edit 2: Looking further at the documentation, Adding VERSION = 2 to the config_flow.py might initiate the async_migrate_entry alone and get your current block running. If no version is present in config_flow I believe it will default to VERSION 1

@Kolbi
Copy link
Contributor Author

Kolbi commented Apr 2, 2024

I'm stuck at the point with migration what I don't know how to access the already (?) saved VIN from the vehicle....

@Kolbi
Copy link
Contributor Author

Kolbi commented Apr 2, 2024

Edit: FWIW -- This time around I tried deleting the duplicate device and I haven't encountered any issues. the "new" device maintains the correct entity id's so existing references aren't impacted.

So worst case no migration but explanation how to delete the old device.

@coreywillwhat
Copy link
Contributor

I'm stuck at the point with migration what I don't know how to access the already (?) saved VIN from the vehicle....

Could you use a listener to wait for Home Assistant to finish starting (EVENT_HOMEASSISTANT_STARTED) and then callback to finish the migration using the vin from AudiConnectAccount?

@cdnninja
Copy link
Collaborator

cdnninja commented Apr 2, 2024

What I did in kia_uvo is the config migration just deleted the entities. That also removed historical data though. It did mean if nothing tied to it everything come up clean. Not perfect but may work. Would make this PR a breaking change so it would be 2.x.

@Kolbi
Copy link
Contributor Author

Kolbi commented Apr 3, 2024

@cdnninja If you want to test use repo: https://github.com/Kolbi/audi_connect_ha/ and install latest version

yeah maybe breaking-change but maybe even without automatic migration or deleting of anything as the data remains only a second empty device appears.

@cdnninja
Copy link
Collaborator

cdnninja commented Apr 4, 2024

Yes I plan to test.

@Kolbi Kolbi changed the title fix: Services.yaml // New Identifier fix:! Services.yaml // New Identifier Apr 5, 2024
@Kolbi Kolbi changed the title fix:! Services.yaml // New Identifier fix!: Services.yaml // New Identifier Apr 5, 2024
@Kolbi
Copy link
Contributor Author

Kolbi commented Apr 5, 2024

#331 as mentioned here maybe we should change the "unique_id"s

I prepared a version but currently I can't test it due to a missing testing enviroment: https://github.com/Kolbi/audi_connect_ha/releases/tag/v2.0.0a6

@cdnninja
Copy link
Collaborator

cdnninja commented Apr 6, 2024

Reviewing this in more depth and testing it out I think two things should happen:

  1. Unique ID should be fully unique, including both VIN and Username. This covers car and account: Multiple accounts #186
  2. We will need to get migration sorted out as it should clean up behind the changes to atleast ensure no duplicate devices.

Let me know thoughts?

@Kolbi
Copy link
Contributor Author

Kolbi commented Apr 6, 2024

Do we have limitation of length for the unique id? Need to check the documentation.
About the migration maybe someone else can take over that part?

@cdnninja
Copy link
Collaborator

cdnninja commented Apr 6, 2024

Sounds good. I'll see if I can get this working. I don't have a ton of time but will give it a go. May take me a bit.

@cdnninja cdnninja marked this pull request as draft April 10, 2024 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants