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

Remove eq3btsmart integration #94698

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

rytilahti
Copy link
Member

@rytilahti rytilahti commented Jun 15, 2023

Proposed change

This PR will remove the eq3btsmart integration that has not been working since the changes introduced to bluetooth connectivbity in the 2022.7 release. Looking at the analytics, most of the users have already moved to use the more feature complete custom component developed by @dbuezas.

I skipped the breaking changes section as there is not much end users can do, nor is there a reason to just deprecate this for a couple of release until removing the dead code. If @dbuezas decides to upstream his integration at somepoint, it is much easier to start from a clean slate.

Feel free to tag this PR accordingly to get it included in the "farewell to the following" section of the relnotes :-)

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

lanrat

This comment was marked as outdated.

@frenck
Copy link
Member

frenck commented Jun 21, 2023

This PR will remove the eq3btsmart integration that has not been working since the changes introduced to bluetooth connectivbity in the 2022.7 release.

I think this is only limited to certain installation types? not all?

Removing it will at least break 23 registered installation that use this, so why would we forcibly break those?

@dbuezas
Copy link
Contributor

dbuezas commented Jun 21, 2023

we forcibly break those?

The integration broke completely quite a while back. The remaining 23 probably just didn't care to remove it from their configuration.yaml

I intend to upstream during the next cold season (northern hemisphere) so me & users can confirm it is still stable.

@frenck
Copy link
Member

frenck commented Jun 21, 2023

The integration broke completely quite a while back.

Nothing explains why or what happened, so at this point, I assume this only affects certain installations. If not, this needs to be clarified.

../Frenck

@rytilahti
Copy link
Member Author

rytilahti commented Jun 21, 2023

Just to add a bit more context, basically, this integration has been broken since the Python 3.10 upgrade and its bluetooth changes (so 2022.7, #73830), so there should be no active users running recent homeassistant versions.

As this integration (nor its upstream lib, I'm "maintainer" for both) has not been properly maintained for years, it's just been chugging along until it didn't. Considering how much homeassistant had evolved since this integration got first added, it was less effort to create a friendly fork to make these devices usable for our users, which is what David did and I am happy for that.

If it's better to keep this "supported" even when it does not work until the rewrite gets mainlined, that is fine by me. I added a couple of relevant links to the PR description, the upstream library has this service announcement pinned to help active users to find the custom component.

@edenhaus edenhaus requested a review from frenck August 7, 2023 07:38
@frenck
Copy link
Member

frenck commented Aug 7, 2023

the upstream library rytilahti/python-eq3bt#70 to help active users to find the custom component.

The Home Assistant project does not support, recommend, or endorse the use of custom integrations. We should thus not recommend such a thing.

../Frenck

@rytilahti
Copy link
Member Author

The Home Assistant project does not support, recommend, or endorse the use of custom integrations. We should thus not recommend such a thing.

That's just a note on the upstream repository and doesn't imply any way that it's official supported by the homeassistant team. I can reword it if it's really necessary, but in my eyes that's a completely unrelated to the PR at hand.

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 12, 2023
@github-actions github-actions bot closed this Nov 19, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2023
@rytilahti
Copy link
Member Author

Still valid.

@rytilahti rytilahti reopened this Nov 20, 2023
@emontnemery
Copy link
Contributor

@rytilahti can you rebase the PR please since it no longer applies cleanly?

@rytilahti
Copy link
Member Author

@emontnemery thanks for the reminder, should be fine after github finishes processing the push 👍

@home-assistant home-assistant unlocked this conversation Nov 27, 2023
@frenck
Copy link
Member

frenck commented Nov 27, 2023

GitHub seems to have some issues right now, so it might take a bit 😬

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @rytilahti 👍

../Frenck

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Nov 27, 2023
@frenck frenck force-pushed the eq3bt/janitor/drop branch from 0b9a488 to 1978a1f Compare November 28, 2023 07:32
@frenck
Copy link
Member

frenck commented Nov 28, 2023

Rebased once more, as the GitHub outage from yesterday, seems to have broken the CI on this PR.

@emontnemery emontnemery merged commit 28a3d36 into home-assistant:dev Nov 28, 2023
49 checks passed
@rytilahti rytilahti deleted the eq3bt/janitor/drop branch November 28, 2023 08:20
@frenck frenck mentioned this pull request Nov 28, 2023
11 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants