Skip to content

Add SensorPush Cloud integration #134223

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
Merged
merged 1 commit into from
Feb 20, 2025
Merged

Add SensorPush Cloud integration #134223

merged 1 commit into from
Feb 20, 2025

Conversation

sstallion
Copy link
Contributor

@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
Merged
Merged
@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

@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
Contributor 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 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
Contributor 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
Contributor 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
Copy link
Contributor 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
Copy link
Contributor Author

I've sanded down a few more edges and made another pass at cleanup. The PR should be ready for review. Thanks in advance!

zweckj
zweckj requested changes Jan 1, 2025
View reviewed changes
@home-assistant home-assistant bot marked this pull request as draft January 1, 2025 11:26
@sstallion sstallion marked this pull request as ready for review January 2, 2025 21:22
@home-assistant home-assistant bot requested a review from zweckj January 2, 2025 21:22
@sstallion
Copy link
Contributor Author

I believe I've addressed all of the review feedback so far. The PR should be ready for another round. Thanks again for the detailed feedback - it is much appreciated!

@sstallion
Copy link
Contributor Author

Rebased and made minor corrections to quality scale for documentation.

@home-assistant home-assistant bot marked this pull request as draft January 18, 2025 12:20
@sstallion
Copy link
Contributor Author

couple small things from my end. Regarding your question what you can do: Just wait 😉 we'll look at it when we can

Will do!

@sstallion sstallion marked this pull request as ready for review January 18, 2025 22:54
@home-assistant home-assistant bot requested a review from zweckj January 18, 2025 22:54
@sstallion
Copy link
Contributor Author

Thanks again @zweckj! I've incorporated your suggestions, updated the branch, and put the PR up for review.

zweckj
zweckj reviewed Jan 22, 2025
View reviewed changes
homeassistant/components/sensorpush_cloud/config_flow.py
except SensorPushCloudError:
LOGGER.exception("Invalid authentication")
errors["base"] = "invalid_auth"
except Exception: # noqa: BLE001
Copy link
Member

Choose a reason for hiding this comment

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

it shouldn't though as you have a LOGGER.exception there
see https://docs.astral.sh/ruff/rules/blind-except/ at the bottom

@sstallion
Copy link
Contributor Author

@zweckj Requested changes have been incorporated into the documentation and this PR. Thanks again for the feedback!

@sstallion sstallion requested a review from zweckj January 23, 2025 01:49
zweckj
zweckj approved these changes Jan 23, 2025
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.

LGTM, let's see if Joost has some final remarks

@sstallion
Copy link
Contributor Author

@joostlek No rush, I just want to keep the PR from going stale.

@ZyberSE
Copy link

ZyberSE commented Feb 1, 2025

@joostlek any chance this PR could make it into the feb release? Would be 🚀

@joostlek
Copy link
Member

joostlek commented Feb 1, 2025

Nope, it's a new feature. I wanted to look at it again this week, but the days before beta were too busy to find time

@ZyberSE
Copy link

ZyberSE commented Feb 1, 2025

Nope, it's a new feature. I wanted to look at it again this week, but the days before beta were too busy to find time

I see. If it's of any help I've been running iterations of this code since last summer with 19 sensors.

@ZyberSE
Copy link

ZyberSE commented Feb 14, 2025

Any chance this could make it into the next release?

@sstallion
Copy link
Contributor Author

Hey @joostlek - it looks like I need to update the documentation PR since this isn't being considered for 2025.02. Do you have an idea of which release I should target? Thanks again!

zweckj
zweckj reviewed Feb 15, 2025
View reviewed changes
@zweckj
Copy link
Member

zweckj commented Feb 15, 2025

Do you have an idea of which release I should target? Thanks again!

Dw about that, we'll fix that if needed

@sstallion
Copy link
Contributor Author

Dw about that, we'll fix that if needed

Sounds good. Thanks!

@sstallion
Copy link
Contributor Author

Thanks @zweckj! I've rebased the branch and pulled in changes from the dev branch. Would it be possible to approve a workflow run for the PR? Thanks again!

joostlek
joostlek reviewed Feb 16, 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.

Last things

homeassistant/components/sensorpush_cloud/config_flow.py
except SensorPushCloudError:
LOGGER.exception("Invalid authentication")
errors["base"] = "invalid_auth"
except Exception: # noqa: BLE001
Copy link
Member

Choose a reason for hiding this comment

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

Ruff indeed does not see that the logger is a logger, I already opened astral-sh/ruff#15473 for that

@sstallion
Copy link
Contributor Author

@joostlek Requested changes have been made. I was able to clean up authentication in the library. Thanks again for the detailed review feedback!

@sstallion
Copy link
Contributor Author

@joostlek Circling back around - is there anything else I can do get this ready to go?

@zweckj zweckj added this to the 2025.3.0b0 milestone Feb 20, 2025
@zweckj zweckj added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Feb 20, 2025
@joostlek joostlek merged commit 73442e8 into home-assistant:dev Feb 20, 2025
46 checks passed
@joostlek
Copy link
Member

@sstallion Be sure to send me a message on discord

@sstallion
Copy link
Contributor Author

@sstallion Be sure to send me a message on discord

Will do. Thanks again!

@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zweckj zweckj zweckj approved these changes

@joostlek joostlek joostlek approved these changes

Assignees
No one assigned
Labels
cla-signed core has-tests integration: sensorpush_cloud new-integration quality-scale second-opinion-wanted Add this label when a reviewer needs a second opinion from another member.
Projects
None yet
Milestone
2025.3.0b0
Development

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