Added arguments to all API calls that accepts auth options.

[Cidan]

This CL adds an optional parameter to all API calls that allows the user to specific authentication parameters on a per-call basis. When authentication parameters are passed in, the authentication parameters in the client it self are ignored.

The rational for this change is described in #128 -- without the ability to handle auth state outside of the client, a user must instantiate and keep track of multiple copies of a client to achieve a high degree of parallelism using this library.

[Cidan]

This CL is a WIP -- while all current tests pass and the changes are backwards compatible, there are no tests for the new functionality yet.

[Cidan]

Okay, it turns out the base auth methods are tested because I moved around some of the token verification code to use the new auth options. PTAL!

[coveralls]

Pull Request Test Coverage Report for Build 1533338711

• 199 of 216 (92.13%) changed or added relevant lines in 32 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.6%) to 91.95%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
helix.go	33	34	97.06%
entitlement_codes.go	2	4	50.0%
extension_secrets.go	0	4	0.0%
authentication.go	40	50	80.0%

Files with Coverage Reduction	New Missed Lines	%
helix.go	1	89.74%

Totals
Change from base Build 1364869930:	-0.6%
Covered Lines:	1245
Relevant Lines:	1354

---

💛 - Coveralls

[nicklaw5]

Looks good @Cidan! We're going to need to add some additional documentation for this change. Did you want to take a stab at that in this PR or in a separate PR?

[bweston92]

Any update on this? I just realised this library is not for concurrent use because of this, 2 validate token requests cannot be made at the same time for example.

[bweston92]

@Cidan I don't suppose you can start adding ctx for first argument and pass it over to the request object? If the function APIs are being broken best to have it break once 👍

[Cidan]

Hi folks,

Sorry, I've been taking time off for the holidays here in the US. I'll finish this up next week when I return.

@bweston92 Currently, the API is backwards compatible -- this is not a breaking change. I don't want to add a context, as this would indeed make it a breaking change.

[bweston92]

Oh I just seen you're doing variadic argument for the options. However you're only using the first one in the slice. That might not be very user friendly. Can I suggest we do something else, where we have an Option type so the API would look like the following:

type Options struct {
	ClientID        string
	ClientSecret    string
	AppAccessToken  string
	UserAccessToken string
	UserAgent       string
	RedirectURI     string
	HTTPClient      HTTPClient
	RateLimitFunc   RateLimitFunc
	APIBaseURL      string
	ExtensionOpts   ExtensionOptions
}

type Option func(o *Options) error

func WithClientID(clientID string) Option {
	return func(o *Options) error {
		o.ClientID = clientID
		return nil
	}
}

Then we can also add one WithTimeout which would look like.

func WithTimeout(readTimeout time.Duration) Option {
	return func(o *Options) error {
		if ok, c := o.HTTPClient.(*http.Client); ok {
			c.Timeout = readTimeout
		}

		return nil
	}
}

[nicklaw5]

I'm open to introducing breaking changes. We'll just need to bump to v3.

I'm also ok with the variadic functions approach. Variadic functions are used widely throughout the Go standard library which is a good point of reference.

[nicklaw5]

@Cidan I've just merged #133. Could you please make sure to update GetExtensionLiveChannels() to a variadic function as part of this PR too? Thanks 🙂

[bweston92]

I would take consideration with the Option type rather then passing a Options struct as a slice and only using the first occurrence. I'm guessing it was done as a hack job.

If you're willing to have breaking changes then add a context too.

I'd be willing to send a PR to @Cidan's fork if they're welcoming of that change.

[Cidan]

I'm absolutely supportive of that change @bweston92 -- you're right that it was added quickly. I wanted to make minimal changes, and at the time it seemed like the easiest path. Feel free to send the PR to my fork, and I'll merge it, along with context if you'd like.

[Cidan]

Sorry, one more thing @bweston92; my only concern with your outlined approach is that it's done correctly in the context of this client. If we're going to implement the whole WithXXX option setters, we should do it correctly, as seen in gRPC DialOptions for example.