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.

Sign up for GitHub

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

Jump to bottom

Add SensorPush Cloud integration #134223

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Add SensorPush Cloud integration #134223

wants to merge 1 commit into from

Conversation

sstallion
Copy link

@sstallion sstallion commented Dec 29, 2024 •
edited
Loading

Proposed change

This PR adds cloud integration for SensorPush devices. It communicates with the publicly available Cloud API using the sensorpush-api and sensorpush-ha Python packages. Care was taken to ensure that presented devices appeared the same as those created by the existing SensorPush integration. A G1 WiFi Gateway is required to make use of the Cloud API.

Note

This PR is a re-submission of #121890, which fixes several issues reported by reviewers. I would like to send a heartfelt apology to @joostlek, @edenhaus, and @frenck - I was caught between dueling schedules and I wasn't able to do the work needed to get it over the finish line. I was finally able to put in the time over the holiday break to make requested changes in addition to improved unit tests, strict typing, async dependencies, and websession injection (whew!)

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 Ruff (ruff format 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.

To help with the load of incoming pull requests:

  1. Remove deprecated yaml import from pyLoad integration #134200
  2. Remove deprecated yaml import from OTP integration #134196

This was referenced Dec 29, 2024
Open
Open
@sstallion sstallion marked this pull request as ready for review December 29, 2024 18:08
zweckj
zweckj requested changes Dec 29, 2024
View reviewed changes
Copy link
Member

@zweckj zweckj left a comment

Choose a reason for hiding this comment

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

Thanks for putting in the effort and going for a second attempt at this! However, I see the libraries are not compliant yet as, sensorpush-api's source code is not public and under an OSI approved license https://developers.home-assistant.io/docs/core/integration-quality-scale/rules/dependency-transparency

homeassistant/components/sensorpush_cloud/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/const.py Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/coordinator.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft December 29, 2024 20:31
@sstallion
Copy link
Author

Thanks for putting in the effort and going for a second attempt at this! However, I see the libraries are not compliant yet as, sensorpush-api's source code is not public and under an OSI approved license https://developers.home-assistant.io/docs/core/integration-quality-scale/rules/dependency-transparency

Thanks! The source code for the Python library is public (I'm the author), however you are correct that the license is not OSI approved. I could use some advice here if you don't mind. OpenAPI generated the original source, which I then modified to work with HA. The license information was generated based on information in the Swagger definition, which references SensorPush's EULA. What have other authors done in this case?

@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch from bae7af7 to 79bf167 Compare December 29, 2024 21:04
@sstallion sstallion marked this pull request as ready for review December 29, 2024 21:05
@home-assistant home-assistant bot requested a review from zweckj December 29, 2024 21:05
@zweckj
Copy link
Member

zweckj commented Dec 29, 2024

I could only find the -ha lib on GitHub. The other one I could only find on PyPi but not in a repo. I guess most will publish to a repo and adapt the license there

@sstallion
Copy link
Author

I could only find the -ha lib on GitHub. The other one I could only find on PyPi but not in a repo. I guess most will publish to a repo and adapt the license there

Huh, that's odd. You should be able to find both libraries on PyPI:

  1. https://pypi.org/project/sensorpush-api/
  2. https://pypi.org/project/sensorpush-ha/

I'll work on re-licensing sensorpush-api in the meantime. I suspect this is just an artifact of OpenAPI.

@zweckj
Copy link
Member

zweckj commented Dec 29, 2024

Yes on PyPi. But that’s not a code repo (like GitHub). You'll need to put the code (+releases & CICD) there as well.

@sstallion
Copy link
Author

Yes on PyPi. But that’s not a code repo (like GitHub). You'll need to put the code (+releases & CICD) there as well.

Ah! That's strange, I'm not sure why the repository link is absent. I'll look into it.

Everything is on GitHub and releases are managed by CI/CD:

  1. https://github.com/sstallion/sensorpush-api
  2. https://github.com/sstallion/sensorpush-ha

@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch 2 times, most recently from 829f651 to 11dee32 Compare December 29, 2024 22:07
@sstallion
Copy link
Author

Hi @zweckj - I've corrected the license terms for sensorpush-api and made a new release. All dependencies use OSI-approved licenses and releases are managed by CI/CD. The PyPI page has also been updated with the correct metadata. Thanks for catching this!

@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch 4 times, most recently from ec21702 to 8678976 Compare December 30, 2024 15:57
@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch 3 times, most recently from 4b0c94f to 5a63c7d Compare January 9, 2025 04:32
@sstallion sstallion marked this pull request as ready for review January 9, 2025 04:33
@home-assistant home-assistant bot requested a review from zweckj January 9, 2025 04:33
@sstallion
Copy link
Author

Latest round of changes in! PR is ready for review - thanks again for the feedback!

joostlek
joostlek requested changes Jan 9, 2025
View reviewed changes
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Hey welcome back, how have you been?

homeassistant/components/sensorpush_cloud/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/config_flow.py Outdated
errors: dict[str, str] = {}
if user_input is not None:
email, password = user_input[CONF_EMAIL], user_input[CONF_PASSWORD]
await self.async_set_unique_id(email, raise_on_progress=False)
Copy link
Member

Choose a reason for hiding this comment

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

also, do you get any tokens from sensorpush? Are we sure we don't have an user id to use

Copy link
Author

Choose a reason for hiding this comment

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

The only token we receive is a short-lived token used for API calls, nothing that would be suitable as a unique ID. SensorPush also relies on email addresses rather than user IDs.

Copy link
Member

Choose a reason for hiding this comment

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

Are they JWTs by any chance?

Copy link
Author

Choose a reason for hiding this comment

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

No, it appears to be completely bespoke.

homeassistant/components/sensorpush_cloud/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/sensorpush_cloud/sensor.py Outdated Show resolved Hide resolved
tests/components/sensorpush_cloud/conftest.py Outdated Show resolved Hide resolved
tests/components/sensorpush_cloud/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/sensorpush_cloud/conftest.py Outdated Show resolved Hide resolved
tests/components/sensorpush_cloud/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/sensorpush_cloud/test_config_flow.py Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft January 9, 2025 09:31
@sstallion
Copy link
Author

Hey welcome back, how have you been?

Good! A little less busy this time around! I hope everyone had a good holiday break.

@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch 7 times, most recently from e78b67a to 19746cd Compare January 11, 2025 04:32
@sstallion sstallion marked this pull request as ready for review January 11, 2025 04:34
@home-assistant home-assistant bot requested a review from joostlek January 11, 2025 04:34
@sstallion
Copy link
Author

@zweckj @joostlek I believe I've incorporated requested changes. Please take a look when you have a moment - thanks again for the detailed feedback!

@home-assistant home-assistant bot marked this pull request as draft January 13, 2025 12:58
@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch 2 times, most recently from 3deb491 to 4720db2 Compare January 13, 2025 20:41
@sstallion sstallion marked this pull request as ready for review January 13, 2025 20:41
@home-assistant home-assistant bot requested a review from joostlek January 13, 2025 20:41
@sstallion
Copy link
Author

sstallion commented Jan 13, 2025 •
edited
Loading

@joostlek Requested changes have been made. Thanks again!

@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch from 4720db2 to 8bc4fcc Compare January 14, 2025 12:27
@sstallion sstallion force-pushed the sstallion/sensorpush_cloud branch from 8bc4fcc to beb5765 Compare January 14, 2025 17:12
@sstallion
Copy link
Author

Apologies, I just rebased the branch without realizing there was a workflow run!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zweckj zweckj Awaiting requested review from zweckj

@joostlek joostlek Awaiting requested review from joostlek

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
cla-signed core has-tests integration: sensorpush_cloud new-integration quality-scale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sstallion @zweckj @joostlek