home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
69.98k stars 29.06k forks source link

Deprecated TheThingsNetwork - replace or remove? #112491

Closed angelnu closed 1 month ago

angelnu commented 4 months ago

The problem

The current implementation for TheThingsNetwork v2 is deprecated. I have implemented support for v3 at https://github.com/angelnu/home_assistant_thethingsnetwork

I would like to either get it added to the core or to HACS. In both cases the old v2 integration needs to be removed (which is anyway useless).

I can do a PR to either remove it from core or to push my new integration - what would the Home Assistent maintainers prefer?

What version of Home Assistant Core has the issue?

dev

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Container

Integration causing the issue

thethingsnetwork

Link to integration documentation on our website

https://www.home-assistant.io/integrations/thethingsnetwork/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

After the decision has been to upgrade the existing core integration, a set of Pull Requests will be needed - this is so that the PRs are not large and take too long to review. This is the current list of such PRs:

- [ ] https://github.com/home-assistant/core/pull/113375 - basics (sensor platform entity, no options menu)
- [x] https://github.com/home-assistant/home-assistant.io/pull/31930 - update documentation to v3
- [ ] TBD - add additional platforms (binary sensor and device tracker)
- [ ] TBD - add options menu
- [ ] TBD - replace debug logs with diagnostics
home-assistant[bot] commented 4 months ago

Hey there @fabaff, mind taking a look at this issue as it has been labeled with an integration (thethingsnetwork) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `thethingsnetwork` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign thethingsnetwork` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


thethingsnetwork documentation thethingsnetwork source (message by IssueLinks)

joostlek commented 4 months ago

We can't just remove one integration and add a new one alongside. If possible we should migrate the integration to adapt to the new way.

angelnu commented 4 months ago

@joostlek - I will see if I can install the old integration and implement a migration. Since the v2 TTN does not longer exists I will not be able to migrate old entities. In addition, the credentials will be different for the new integration so the migration will need to flag the integration as pending additional settings.

Btw- I expect that the number of active users of this integration are using my new custom one as it has been 2 years since the old one does not work.

joostlek commented 4 months ago

I saw the message on the documentation page.

I mean, if there is no way of migrating properly because of different credentials etc, we can decide on doing a harder way of setting up the integration. Since I don't have this brand and I never heard of it, I would like to have more context before we can convince people that this is the way forward.

What is the old way of authenticating and how does it differ from the new one? If an old entity had an entity, I would imagine a new one would also show it, is there anything that is there from the old API that we can use in the new API?

And maybe the integration was deprecated too long ago (I mean 2 years is quite a lot) so I can understand if you can't find out how it used to work or compare it properly, but we should do our due diligence before tackling this problem.

angelnu commented 4 months ago

The authentication is still the same: and application_id and a token. What has changed is that the old application_id and tokens are not longer valid when moving from TheThinksNetwork (TTN) v2 to v3: the team running TTN deployed v3 in parallel to v2 and gave the users several months to move to v3 before switching off v2. They did not migrate the environments since the changes were very large.

My concern with the migration is less about not doing it that my ignorance if there is a way in Home Assistant to mark an integration as requiring additional configuration parameters after a migration. I expect there is a way as I recall seeing integrations in my HA with a warning, asking to be reconfigured.

joostlek commented 4 months ago

TTN is currently a YAML based integration, before we can add new features and or extend it, we should migrate it to a config flow. Usually we import the YAML integration and create a config entry out of it, but if the old YAML isn't usable, there's no point in importing that.

Your integration has a config flow and I think that's a good startpoint.

Let me ask some people to make a decision over this and then we can plan the next steps.

joostlek commented 4 months ago

Oh and keep in mind, for an integration in core, you should publish your device/service specifics in a separate library to PyPi.

angelnu commented 4 months ago

Thanks @joostlek

I am aware that the integration will require some massaging to fulfill the guidance to be merged in core - I am willing to do the changes if there is interest to replace the old integration.

I havey placed the code that should get into a separate library in an extra file at https://github.com/angelnu/home_assistant_thethingsnetwork/blob/master/custom_components/thethingsnetwork/TTN_client.py

I am also going through the Ruff warnings, trying to address them as I also expect this to be needed.

fabaff commented 4 months ago

According to Analytics (https://www.home-assistant.io/integrations/thethingsnetwork) there are only a few installations which are using the integration or have it still present. From my point of view I highly doubt that many users would profit from preserving the existing data with the help of a migration path.

Those who are still interested in the historic data might be able to do the migration of the sensor data manually anyways as LoRa is still a "nerd-thing" and most applications are not consumer-grade.

joostlek commented 4 months ago

Definetly, but we also made a promise to our users to do that with best effort, so if we can do that, we will, and otherwise we won't

joostlek commented 4 months ago

Aight, I got a response and we can practically replace the integration. We should however create an issue for the people that still have the YAML setup that they should remove their config. But that's about it of what should remain for the old integration.

angelnu commented 4 months ago

Thanks @joostlek - I will then work towards a PR to replace the current integration.

I am able to do a migration from the configuration.yaml - it does then show the (new) integration in ConfigEntryAuthFailed until the user enters the new credentials.

I am progressing in cleaning up the code and split the library code into a separate module. I think I should have the PR ready this week.

joostlek commented 4 months ago

I think we should raise an issue instead. I can imagine that when you're not using the service anymore that you just ignore the reauth and then forget to take action on the issue that is created to point users at removing the YAML

angelnu commented 4 months ago

gotcha - it would then raise an issue if I detect entries in the configuration.yaml Is there a recommended error state for the integration you would recommend?

joostlek commented 4 months ago

Raise an issue in the issue registry. But just create the PR, when the time is there I'll point you to examples and we can make it go from there