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

tariff/octopusenergy: Support API Keys for tariff data lookup #11555

Closed

Conversation

duckfullstop
Copy link
Contributor

This PR adds support for the apikey option on the octopusenergy 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.

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.
@duckfullstop duckfullstop marked this pull request as ready for review January 20, 2024 15:41
@duckfullstop
Copy link
Contributor Author

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

@duckfullstop duckfullstop requested a review from andig January 20, 2024 15:42
@andig andig added the enhancement New feature or request label Jan 23, 2024
tariff/octopus.go Outdated Show resolved Hide resolved
var restQueryUri string

// If ApiKey is available, use GraphQL to get appropriate tariff code before entering execution loop.
if t.apikey != "" {
Copy link
Member

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.

Copy link
Contributor Author

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 🙏

Copy link
Member

Choose a reason for hiding this comment

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

ping ;)

Copy link
Contributor Author

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.

@andig
Copy link
Member

andig commented Jan 23, 2024

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?

@duckfullstop
Copy link
Contributor Author

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 <cpuidle@gmx.de>
@andig
Copy link
Member

andig commented Jan 28, 2024

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?

@andig andig marked this pull request as draft January 28, 2024 15:52
@duckfullstop
Copy link
Contributor Author

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. 🙏

@github-actions github-actions bot added the stale Outdated and ready to close label Mar 13, 2024
@github-actions github-actions bot closed this Mar 18, 2024
@naltatis
Copy link
Member

naltatis commented Apr 3, 2024

@duckfullstop @andig GitHub bot closed this because of stale. Still relevant? Asking because of the corresponding docs PR evcc-io/docs#509

@duckfullstop
Copy link
Contributor Author

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

@andig
Copy link
Member

andig commented Apr 5, 2024

Sorry for the delay, life's been happening 🫨

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.

@duckfullstop
Copy link
Contributor Author

duckfullstop commented Apr 26, 2024

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...

@andig andig removed the stale Outdated and ready to close label Apr 26, 2024
@andig
Copy link
Member

andig commented Apr 27, 2024

@duckfullstop please create new PR- GH doesn't allow me to reopen this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants