-
-
Notifications
You must be signed in to change notification settings - Fork 684
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
tariff/octopusenergy: Support API Keys for tariff data lookup #11555
tariff/octopusenergy: Support API Keys for tariff data lookup #11555
Conversation
Initial work, more to be done here, committing as I've been working on this most of the evening! Splits GraphQL and Rest out into separate packages. Rest is still required even with GraphQL being available as GraphQL has a lot of restricted endpoints.
validators, etc
Tidied this up a bit and I see no glaring holes that would prevent it being merged, going to work on Intelligent separately I think |
var restQueryUri string | ||
|
||
// If ApiKey is available, use GraphQL to get appropriate tariff code before entering execution loop. | ||
if t.apikey != "" { |
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.
Shouldn't this part be moved to the tariff init as its a one-time setup efforts? Also saves storing the api key and client.
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.
On mobile but I'll inspect this in more detail once I'm in front of a laptop later 🙏
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.
ping ;)
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.
Sorry for the delay, life's been happening 😭
You've a fair point here - was trying to think how best to handle these two ways of setting up. Refactoring the rest
module would probably be a better solution, generating the API URL and holding onto it in this execution loop seems a bit wrong.
The above being said, see my other comment on refresh tokens.
I'm a bit lost why we should be doing this, at least at this time. There is currently no additional functionality for users (that we're targeting). Should we leave this until there is additional benefit for the complexity? |
For the end user, getting the API key is surprisingly easier than getting the tariff code & region - the API key just requires navigating to a page while signed in, while the tariff code & region is buried in the bill (and requires decoding), and indeed some guides suggest querying the API manually to get it! I figured shipping this now gets users using the new format as soon as possible, especially as my availability to write features deriving from it is limited - I have started on intelligent support in another branch but it's very early days 😵💫 |
Co-authored-by: andig <[email protected]>
From what I understood, graphql is only ever needed during init. In that case- can we remove all the refresh token complexity and the like? |
I've implemented GraphQL with refresh tokens as future functionality will depend on the ability to query repeatedly (and as a result the ability to refresh). I appreciate it looks a bit redundant in this implementation, but it's forward facing. 🙏 |
@duckfullstop @andig GitHub bot closed this because of stale. Still relevant? Asking because of the corresponding docs PR evcc-io/docs#509 |
Sorry for the delay, life's been happening 🫨 This can be merged as previously discussed, but I don't presently have the time to work on adding the intelligent slot functionality. The ability for end users to use an API key instead of manually deriving their tariff key would, IMO, make things easier to configure. If the feeling is that this adds unnecessary complexity, then probably best to stale both of these PRs for the time being. |
Oh, that again ;). Happy to merge if we can add a yaml template. Right now (and in future via the docs) it's not really accessible at all. |
Suggest un-staling per discussion in #13474 - though not sure if I've somehow managed to break this PR, all kinds of strangeness is happening in my Github now with this branch... |
@duckfullstop please create new PR- GH doesn't allow me to reopen this one. |
This PR adds support for the
apikey
option on theoctopusenergy
tariff provider. Using an API key allows for automatic detection of the user's relevant tariff, and also allows for some potentially exciting functionality down the road such as user-specific tariff (and load shedding?) flexibility.Initial work, lots more to be done here, committing as I've been working on this most of the evening!
Splits a new GraphQL provider and Rest out into separate packages. Rest is still required even with GraphQL being available as GraphQL has a lot of restricted endpoints / retains backwards compatibility.
Further PR will add support for handling Intelligent Octopus slots as discussed in slack.