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

Add weheat core integration #123057

Merged
merged 34 commits into from
Sep 6, 2024

Conversation

jesperraemaekers
Copy link
Contributor

Breaking change

Proposed change

This will integrate the heat pumps of Weheat. We've started implementing only 3 temperature sensors to keep the PR size small (as small as it can be for a new integration). More sensors and a controllable interface will follow.

Some notes on the checklist below:

  • Tests are added for the config flow, but not other parts. We plan on adding more capabilities soon and will put effort i increasing the test coverage
  • There is no changelog for the dependency, it was newly released for this integration.
  • I am sorry for not reviewing other PRs, but feel like getting for feedback on this first PR to establish if my baseline matches with the standards you require.

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:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @jesperraemaekers,@kjell-van-straaten

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@jesperraemaekers jesperraemaekers changed the title Feature/weheat integration Add weheat core integration Aug 2, 2024
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @jesperraemaekers,@kjell-van-straaten

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

Now you almost sound like a frontender :P

@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

And even if the refresh of the token is short, it shouldn't cause a reauth. I think a possible solution could be that they invalidate at the same time. The moment session.async_token_something_valid is called and its not valid anymore, it will use the refresh token, and if that fails it starts reauth

@jesperraemaekers
Copy link
Contributor Author

Now you almost sound like a frontender :P

This is already long ways from the firmware i normally work on, trying to fit in :D

I did see before that the session of the logged in user in keycloak is pretty short.

@barryvdh
Copy link
Contributor

barryvdh commented Sep 4, 2024

Id di notice that de refresh token validity is not that long, but indeed you should not have to re-auth every few days. I'll check in with out back-end guys managing the oath. aiohttp is available in the openai generator we use for the API, let me check if that is also pydantic v1 or does not use it.

I use the 'offline' scope for my rest api integration, that has never expired. Maybe you can use that for this oauth flow also?

@jesperraemaekers
Copy link
Contributor Author

Id di notice that de refresh token validity is not that long, but indeed you should not have to re-auth every few days. I'll check in with out back-end guys managing the oath. aiohttp is available in the openai generator we use for the API, let me check if that is also pydantic v1 or does not use it.

I use the 'offline' scope for my rest api integration, that has never expired. Maybe you can use that for this oauth flow also?

I did now, would you be so kind to see if it work better?

@barryvdh
Copy link
Contributor

barryvdh commented Sep 4, 2024

Is there a way to see the JWT token? Then you can check the expiration date

@joostlek
Copy link
Member

joostlek commented Sep 4, 2024

check /config/.storage there's a file in there with config_entries in the name. It has all your config and thus the access token

@barryvdh
Copy link
Contributor

barryvdh commented Sep 5, 2024

Okay, new update has no expiration, the previous one had 30 days. Which makes sense given you provided the keys a month ago :)

Old key:

   "expires_in": 300,                        
    "refresh_expires_in": 2522164,                
    "scope": "api.access.odata profile email",
    "expires_at": 1725529324.5112088    

New key:

    "expires_in": 300,                                                                            
    "refresh_expires_in": 0,     
    "scope": "openid api.access.odata profile offline_access email",                                                                                                                                                                                                                       
    "expires_at": 1725531907.6238155  

Few notes:

@jesperraemaekers
Copy link
Contributor Author

(also, why don't we use aiohttp? The library is approached async)

The weheat library is 90% just generated code from a API specification. the generator just does not happen to use aiohttp for the API client.

@jesperraemaekers jesperraemaekers marked this pull request as ready for review September 5, 2024 13:10
@home-assistant home-assistant bot requested a review from joostlek September 5, 2024 13:10
@home-assistant home-assistant bot marked this pull request as draft September 5, 2024 13:15
@jesperraemaekers
Copy link
Contributor Author

We checked and found out that the access_token lifetime was set to 1 minute for the HomeAssistantAPI client id, which does not help the authentication at all. This is now corrected.

@jesperraemaekers jesperraemaekers marked this pull request as ready for review September 6, 2024 07:43
@home-assistant home-assistant bot requested a review from joostlek September 6, 2024 07:43
@joostlek
Copy link
Member

joostlek commented Sep 6, 2024

LOL

@joostlek
Copy link
Member

joostlek commented Sep 6, 2024

Send you a message on discord

@joostlek joostlek merged commit dfcfe78 into home-assistant:dev Sep 6, 2024
41 of 42 checks passed
@joostlek
Copy link
Member

joostlek commented Sep 6, 2024

Oh and nice seeing @barryvdh here. I used quite some laravel stuff you made back in the days :)

Gigatrappeur pushed a commit to Gigatrappeur/ha-core that referenced this pull request Sep 7, 2024
* Add empty weheat integration

* Add first sensor to weheat integration

* Add weheat entity to provide device information

* Fixed automatic selection for a single heat pump

* Replaced integration specific package and removed status sensor

* Update const.py

* Add reauthentication support for weheat integration

* Add test cases for the config flow of the weheat integration

* Changed API and OATH url to weheat production environment

* Add empty weheat integration

* Add first sensor to weheat integration

* Add weheat entity to provide device information

* Fixed automatic selection for a single heat pump

* Replaced integration specific package and removed status sensor

* Add reauthentication support for weheat integration

* Update const.py

* Add test cases for the config flow of the weheat integration

* Changed API and OATH url to weheat production environment

* Resolved merge conflict after adding weheat package

* Apply suggestions from code review

Co-authored-by: Joost Lekkerkerker <[email protected]>

* Added translation keys, more type info and version bump the weheat package

* Adding native property value for weheat sensor

* Removed reauth, added weheat sensor description and changed discovery of heat pumps

* Added unique ID of user to entity

* Replaced string by constants, added test case for duplicate unique id

* Removed duplicate constant

* Added offline scope

* Removed re-auth related code

* Simplified oath implementation

* Cleanup tests for weheat integration

* Added oath scope to tests

---------

Co-authored-by: kjell-van-straaten <[email protected]>
Co-authored-by: Joost Lekkerkerker <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
@jesperraemaekers jesperraemaekers deleted the feature/weheat_integration branch September 9, 2024 09:57
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