Include the version of `go-github` in `User-Agent` headers sent to the GitHub API by timrogers · Pull Request #2403 · google/go-github

timrogers · 2022-07-03T16:19:54Z

👋🏻 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 · 2022-07-03T16:19:58Z

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.

timrogers · 2022-07-03T16:23:16Z

github/github.go

 )

 const (
+	packageVersion = "45.2.0"


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.

timrogers · 2022-07-03T16:23:31Z

github/github.go

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


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

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

timrogers · 2022-07-03T16:48:32Z

github/github.go

 	// User agent used when communicating with the GitHub API.
 	UserAgent string

+	PackageVersion string


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!

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

timrogers · 2022-07-03T16:49:22Z

github/github_test.go

 		t.Errorf("NewRequest() User-Agent is %v, want %v", got, want)
 	}
+
+	if !strings.Contains(userAgent, c.PackageVersion) {


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?

I think this is sufficient, thanks.

codecov · 2022-07-03T19:53:13Z

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.

gmlewis

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

gmlewis · 2022-07-03T19:57:04Z

github/github.go

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


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

gmlewis · 2022-07-03T19:59:07Z

github/github.go

 	// User agent used when communicating with the GitHub API.
 	UserAgent string

+	PackageVersion string


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

gmlewis · 2022-07-03T20:00:07Z

github/github.go

 	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}


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

gmlewis · 2022-07-03T20:00:31Z

github/github_test.go

 		t.Errorf("NewRequest() User-Agent is %v, want %v", got, want)
 	}
+
+	if !strings.Contains(userAgent, c.PackageVersion) {


I think this is sufficient, thanks.

timrogers

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

gmlewis

Thank you, @timrogers !
LGTM.

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

raynigon

LGTM

raynigon · 2022-08-13T21:38:31Z

CONTRIBUTING.md

 [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.


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 · 2022-08-13T22:21:51Z

Thank you, @raynigon !
Merging.

timrogers added 2 commits July 2, 2022 22:21

timrogers commented Jul 3, 2022

View reviewed changes

gmlewis requested changes Jul 3, 2022

View reviewed changes

timrogers added 3 commits July 3, 2022 23:11

Rename userAgent constant to defaultUserAgent

75465e1

Export current package version as CurrentVersion constant

8495ae5

Drop PackageVersion field from Client struct

f65d26e

timrogers commented Jul 3, 2022

View reviewed changes

github/github.go Outdated Show resolved Hide resolved

timrogers requested a review from gmlewis July 3, 2022 22:13

timrogers added 2 commits July 4, 2022 09:30

Rename CurrentVersion to Version

c963102

Add reminder to bump version to CONTRIBUTING.md

7279ad7

gmlewis approved these changes Jul 4, 2022

View reviewed changes

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

raynigon approved these changes Aug 13, 2022

View reviewed changes

gmlewis merged commit f2d99f1 into google:master Aug 13, 2022

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Aug 19, 2022

gmlewis mentioned this pull request Aug 19, 2022

Bump version of go-github library to v46 #2437

Merged

Conversation

timrogers commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla bot commented Jul 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timrogers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

raynigon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Aug 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timrogers commented Jul 3, 2022 •

edited

Loading

codecov bot commented Jul 3, 2022 •

edited

Loading