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

Add a helper for generating Consul's user-agent string #4013

Merged
merged 3 commits into from
Jun 1, 2018
Merged

Add a helper for generating Consul's user-agent string #4013

merged 3 commits into from
Jun 1, 2018

Conversation

sethvargo
Copy link
Contributor

This is not currently used, but it should be. Refs hashicorp/go-discover#33

/cc @pearkes

@mkeeler
Copy link
Member

mkeeler commented Apr 6, 2018

@sethvargo Thanks for the PR. Do you have ideas already for places in the consul code where you think this new function should be used?

@sethvargo
Copy link
Contributor Author

Right now, nowhere 😄 Once hashicorp/go-discover#33 is merged, it'll be when Consul makes external API calls.

I would also like if Consul set its user agent when invoking health checks, but there's already a hard-coded user-agent string, and I don't know the implications of changing that. Ideally (external) services would like to know more about Consul and the chain, but I don't know if we can change that for BC.

@pearkes pearkes modified the milestones: 1.0.7, Next Apr 9, 2018
@pearkes
Copy link
Contributor

pearkes commented Apr 9, 2018

Yeah I think we should think separately about the heath check change for the reasons you mention but we should be able to get this and the corresponding go-discover change in for the release following 1.0.7.

@pearkes pearkes modified the milestones: Next, 1.1.0 Apr 17, 2018
@sethvargo
Copy link
Contributor Author

Hey @pearkes - I updated this PR to pull in the new lib and connect it all at once. I'm not sure why there are so many changes to vendor. I ran govendor update github.com/hashicorp/go-discover/... and it seems to have pulled in the world. I'm guessing those are all transitive dependencies.

@pearkes
Copy link
Contributor

pearkes commented May 4, 2018

Thanks @sethvargo -- yeah I think this will also fix #4082, FYI @preetapan.

@pearkes pearkes modified the milestones: 1.1.0, Upcoming May 11, 2018
@pearkes
Copy link
Contributor

pearkes commented May 25, 2018

@sethvargo thanks for your patience on this! We got the vendor changes in a separate change, so do you mind rebasing which should just include the changes required to set the user agents in the go-discover clients?

@sethvargo
Copy link
Contributor Author

Updated @pearkes. I'm not sure why there's still some whitespacing diffs on three of the vendor files, but I also don't know how to fix it. Hopefully that's okay

Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

Thanks!

@pearkes pearkes merged commit aa1c993 into hashicorp:master Jun 1, 2018
@sethvargo sethvargo deleted the sethvargo/user_agent branch June 1, 2018 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants