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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include the version of go-github in User-Agent headers sent to the GitHub API #2403

Merged
merged 7 commits into from Aug 13, 2022

Conversation

timrogers
Copy link
Contributor

@timrogers timrogers commented Jul 3, 2022

馃憢馃徎 Hi there! I'm Tim, and I'm a Product Manager at GitHub. To allow us to better understand how people are using our API, it's super helpful to know what versions of SDKs like go-github they are using.

At the moment, this package sends the User-Agent "go-github" in requests, but it doesn't say what version is being used.

This updates the User-Agent header to include that information, whilst still maintaining the ability for people to choose their own user agent by setting the UserAgent field on Client.

It also exports a Version constant, so people can use that information - for example to construct custom User-Agent headers.

This will mean that someone has to update the Version constant with the new version when it is incremented.

This is the first ever Go I've written, so I won't be upset if you tell me that I've done this in a terrible way 馃槈

This exposes the current version of the `go-github` package
(e.g. `"45.2.0"`) as a field on the `Client` struct.

This will allow users of `go-github` to access the current version
of the package, for example if they want to construct a custom
`User-Agent` header including it.

This change means that, before releasing tagging and releasing a
new version of the package, this string constant will have to be
updated.
鈥 header

This updates the default request header sent by the module to
include the current version, e.g. `go-github/45.2.0`.

This makes it easier for GitHub to track usage of this SDK and
how users are upgrading between versions.

Users can continue to overwrite this default user agent with
their own string by setting the `UserAgent` property on the
`Client`.
@google-cla
Copy link

google-cla bot commented Jul 3, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

github/github.go Outdated
@@ -28,9 +28,11 @@ import (
)

const (
packageVersion = "45.2.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find this defined anywhere else in the package code, so I added it here. I'm happy to move it elsewhere, or access it in some other way if there are good options out there.

github/github.go Outdated
defaultBaseURL = "https://api.github.com/"
uploadBaseURL = "https://uploads.github.com/"
userAgent = "go-github"
userAgent = "go-github" + "/" + packageVersion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of renaming this to defaultUserAgent, which is more representative of what it is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that sounds good, @timrogers - let's also please sort these contant names within this group of 4 if you don't mind.

github/github.go Outdated
@@ -167,6 +169,8 @@ type Client struct {
// User agent used when communicating with the GitHub API.
UserAgent string

PackageVersion string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that exposing this on the Client would help with (a) people constructing customer User-Agent headers and (b) me testing the default User-Agent!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, you can kill two birds with one stone just by exporting the packageVersion constant above by making it PackageVersion and problem solved.

t.Errorf("NewRequest() User-Agent is %v, want %v", got, want)
}

if !strings.Contains(userAgent, c.PackageVersion) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there somewhere else I should add a test to help confirm that this is being added to all requests? Or do you think this is sufficient?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is sufficient, thanks.

@codecov
Copy link

codecov bot commented Jul 3, 2022

Codecov Report

Merging #2403 (7279ad7) into master (fd22ee9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2403   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         119      119           
  Lines       10546    10546           
=======================================
  Hits        10342    10342           
  Misses        140      140           
  Partials       64       64           
Impacted Files Coverage 螖
github/github.go 97.80% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update fd22ee9...7279ad7. Read the comment docs.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Great idea, @timrogers !
Just a couple minor tweaks, please, then we should be ready for a second LGTM+Approval before merging.

github/github.go Outdated
defaultBaseURL = "https://api.github.com/"
uploadBaseURL = "https://uploads.github.com/"
userAgent = "go-github"
userAgent = "go-github" + "/" + packageVersion
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that sounds good, @timrogers - let's also please sort these contant names within this group of 4 if you don't mind.

github/github.go Outdated
@@ -167,6 +169,8 @@ type Client struct {
// User agent used when communicating with the GitHub API.
UserAgent string

PackageVersion string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, you can kill two birds with one stone just by exporting the packageVersion constant above by making it PackageVersion and problem solved.

github/github.go Outdated
@@ -301,7 +305,7 @@ func NewClient(httpClient *http.Client) *Client {
baseURL, _ := url.Parse(defaultBaseURL)
uploadURL, _ := url.Parse(uploadBaseURL)

c := &Client{client: httpClient, BaseURL: baseURL, UserAgent: userAgent, UploadURL: uploadURL}
c := &Client{client: httpClient, BaseURL: baseURL, PackageVersion: packageVersion, UserAgent: userAgent, UploadURL: uploadURL}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this would no longer be needed since the PackageVersion constant is now exported and available.

t.Errorf("NewRequest() User-Agent is %v, want %v", got, want)
}

if !strings.Contains(userAgent, c.PackageVersion) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is sufficient, thanks.

Copy link
Contributor Author

@timrogers timrogers left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback! I've addressed those comments and I think it's looking better now. Plus I learned about exported constants, which is great 馃 猬嗭笍

github/github.go Outdated Show resolved Hide resolved
@timrogers timrogers requested a review from gmlewis July 3, 2022 22:13
gmlewis
gmlewis approved these changes Jul 4, 2022
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @timrogers !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this report before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Jul 4, 2022
Copy link
Contributor

@raynigon raynigon left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -127,3 +127,5 @@ this][modified-comment].
[git-aliases]: https://github.com/willnorris/dotfiles/blob/d640d010c23b1116bdb3d4dc12088ed26120d87d/git/.gitconfig#L13-L15
[rebase-comment]: https://github.com/google/go-github/pull/277#issuecomment-183035491
[modified-comment]: https://github.com/google/go-github/pull/280#issuecomment-184859046

**When creating a release, don't forget to update the `Version` constant in `github.go`.** This is used to send the version in the `User-Agent` header to identify clients to the GitHub API.
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be automated. Since the version constant is currently only used for the user agent, an error should not be a big problem, but if somebody depends on the constant this could lead to problems along the way.
Unfortunatly the release process in this project is not automated yet. If it gets automated in the future we need to include this constant in there.

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 13, 2022

Thank you, @raynigon !
Merging.

@gmlewis gmlewis merged commit f2d99f1 into google:master Aug 13, 2022
10 checks passed
@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 19, 2022
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.

None yet

3 participants