added ecobee.New(), Options and AuthCache

[jkowalski]

This allows persisting OAuth2 tokens in storage other than files,
for example etcd or a database.

[rspier]

Thank you! I think this is good, and could be convinced to merge it as is if you don't like the ideas below.
I'm thinking about the NewClient interface.

Instead of NewClientWithAuthCache, what do you think about NewClientWithOptions that takes an *Options struct, of which the (only, at the moment) member is a TokenSource?

This would also be an opportunity to pull out the hardcoded context.Background()

type Options struct {
TokenSource *oauth2.TokenSource
}

func NewClientWithOptions(ctx context.Context, clientID string, opt *Options) { }

NewClientWIthOptions isn't my favorite name, but since NewClient is taken, I don't have a better idea beyond just New.

WDYT?

-R

[jkowalski]

Implemented New() which takes context.Context and Options + deprecated NewClient()

[rspier]

This is never used. Was this supposed to check clientID? I think I was mixing OAuth terminology with ecobee terminology.

Also, this will fail whenever NewClient is used, because it doesn't set it.

Looking at the ecobee portal, it's now just called an API key. Since this is always required, I think it can be a normal parameter.

[jkowalski]

Both parameters in Options are currently required, I think it's cleaner to pass them all through Options (except ctx which would look weird in a struct).

Removed clientID parameter here, renamed to appKey everywhere.

[rspier]

AuthCache doesn't actually need to be required, although it would be kind of weird to not provide it.

I've been staring at the oauth2 library and wondering if we've built the right interface. (Since it's hard to change later.)

golang/oauth2#84 has given me some ideas, but I can't just copy that code.

I may be overthinking this.

[jkowalski]

Ok, I did some experimentation and it turns out most of the ecobee.tokenSource functionality can be replaced with golang.org/x/oauth2 package which also makes it easy to get OAuth2 HTTP client.

Given that, if we added ecobeekeeper.New(hc *http.Client), I can make OAuth2 work externally with relatively little effort:

cfg := &oauth2.Config{
  Endpoint: oauth2.Endpoint{
    TokenURL: "https://api.ecobee.com/token?client_id=3KPS9bjYiH....0MIZR",
  },
}

// load token from DB once at startup
var tok *oauth2.Token = loadTokenFromDB(db)
var ts *oauth2.TokenSource = cfg.TokenSource(ctx, tok)
// wrapper around TokenSource that saves tokens in DB whenever they change
ts = cachingTokenSource(ts, db) 
cli := ecobee.New(oauth2.NewClient(ctx, ts))

What do you think?

Internally we can build caching wrapper around TokenSource that writes to a file to support legacy API.

Also, we'd need to do something about intiial token fetching, but I think a simple command line tool like https://gist.github.com/jkowalski/20736a32a39ba85d9f7e630b6125e2f6 should do

[rspier]

Hi Jarek,
Sorry for not making any progress on this myself, I've been getting home from work and not wanting to spend more time with a computer.

I like this direction. It gets rid of a bunch of code and simplifies things.

Do you need to put the client_id in the TokenURL, or can you just put it in the oauth2.Config directly?

I think we can re-use the TokenSource interface for a lot of the wrappers and it will be pretty clean. Some of the examples in the bug linked before do something similar.

Instead of the initial fetching that's in a separate command line tool, it could be a library so that CLI tools could just use it. Maybe it could be a TokenSource?

One thing we don't support right now is the web based oauth flow (which is supported, you configure it in the ecobee portal.)

Instead of 100% backwards compatibility, it might be time to declare a v2 API and start using go mod and semantic versioning. https://github.com/golang/go/wiki/Modules. I'm open to that.

Thanks!

[jkowalski]

Sounds like great ideas. I'm going to close this PR. I technically was able to use unmodified code + the tool from my GIST to load/store token in etcd and it works great. I'd love to collaborate on v2 efforts and port my usage to v2 when ready.

[rspier]

Thanks! See inline comment.

[rspier]

I'm reopening this PR so I can remove it from my INBOX but not lose track of it :) I don't want to forget about it, as I promised I'd give a serious look to improving the API. So even if we're not merging this exact code, I want to keep the discussion around.