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
Store access token in entry for Fyta #116260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a minor version config entry migration for this. This way you create a central place where you update the config entry.
Thank you for the hint, done. Is it then save to remove the following code (respectively change it to
|
Minor version, not a major version. Minor versions keep working after a restore of a version before this. Major versions don't, and this change doesn't affect the working on old versions |
I think you can always expect an expiration after this migration |
Yes, sure. But this was not my question. Expiration is catched in coordinator. Question was, if I can assume correct expiration value in config entry or if I should leave the saveguard there. |
What safeguard are you exactly talking about? In the code of your previous comment I see 2 about expiration not being in entry data, maybe I'm missing it |
Or if I should just use |
But that's what I meant, after the migration is run, you can expect expiration to be there in entry.data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering 2 things. What's the value of expiration? The Epoch? Isn't that always UTC?
How long is a token valid? Can we get a new one without username/password?
The API returns a validity in seconds (cf. below), which is then added to the current time in fyta_cli:
Token is valid 5184000 seconds, i.e. 60 days. According to my understanding the renewal requires username and password. |
Ah, in that case I agree that this is a good change. |
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
I think it would make sense to include this in 2024.5 because it will prevent doing a request every time HA boots/the integration reloads. I only see benefit in pulling this into the beta, to avoid unnecessary calls. |
* save access_token and expiration date in ConfigEntry * add MINOR_VERSION and async_migrate_entry * shorten reading of expiration from config entry * add additional consts and test for config entry migration * Update homeassistant/components/fyta/coordinator.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * Update homeassistant/components/fyta/__init__.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * omit check for datetime data type * Update homeassistant/components/fyta/__init__.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> * Update homeassistant/components/fyta/coordinator.py Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> --------- Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the comments in a new PR. Thanks!
Just opened a respective PR #116433. |
Proposed change
Save the access_token and its expiration received from the API in ConfigEntry an load it in
async_setup_entry
to reduce unnecessary login-calls to the API.Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.ioIf 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 torequirements_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: