Provide a way to set custom user agents for clients #33
Conversation
There was a problem hiding this comment.
Thank you for the contribution, @sethvargo! Left a few comments, mostly just curious if we need to make as many changes as we are to support this.
| @@ -1,19 +1,79 @@ | |||
| { | |||
| "comment": "", | |||
| "ignore": "test", | |||
There was a problem hiding this comment.
Do you mind formatting this file? I believe this was previously done manually. We don't have a Makefile or similar on this project, so there's no obvious way of knowing it, but should clean up the diff.
$ go get -u github.com/magiconair/vendorfmt/cmd/vendorfmt
$ vendorfmt
There was a problem hiding this comment.
I'm also wondering if we can drop the softlayer updates here too.
There was a problem hiding this comment.
Formatted. We have to keep the updates. The ability to set a custom user agent wasn't available in the vendored version.
There was a problem hiding this comment.
@sethvargo Given softlayer isn't one of the providers we run regular acceptance tests against I'd rather drop these changes to it -- it is ok if it does not send the user agent at this time.
discover.go
Outdated
| type Option func(*Discover) error | ||
|
|
||
| // NewDiscover creates a new discover client with the given options. | ||
| func NewDiscover(opts ...Option) (*Discover, error) { |
There was a problem hiding this comment.
If I'm reading this right you're setting up a new way to initialize go-discover. Is it possible to limit the changes to fit with the existing APIs? If not, can you describe the use-case and example in Consul (I believe you're planning on utilizing this over there, per your comment)?
There was a problem hiding this comment.
Also I'd name this New, not NewDiscover, since the package is already discover. This is just some guideline from "writing effective go".
There was a problem hiding this comment.
Renamed to New. Thanks for the feedback.
The existing API doesn't change and is not obsoleted. However, I didn't want to make UserAgent a publicly accessible field on the Discover struct because it would introduce race conditions.
The API current is:
d := discover.Discover{}That API will continue to function, but it's not very flexible, so I adopted the "options pattern" to allow for this:
d := discover.New(discovery.WithUserAgent("consul/1.0"))There was a problem hiding this comment.
To be clearer: there exists a sad race condition in the current implementation. Since Providers is a public field on the struct, a user can set Providers to something, call a command, and then alter the field. However, since the init is done in a once, those changes won't take place.
|
Ping @pearkes @mitchellh |
This adds a new interface that providers can implement that sets a user agent string. This is useful for upstream calling libraries.
|
Removed the functionality from Softlayer @pearkes |
|
Thanks @sethvargo! |
This adds a new interface that providers can implement that sets a user agent string. This is useful for upstream calling libraries.
/cc umm @pearkes - not sure who is managing this library anymore. Once this is merged, we can update Consul and Nomad upstream.