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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7152610
Expose the package version on the `Client` (e.g. `"45.2.0")
timrogers Jul 2, 2022
3157af0
Include the version of the module in the default `User-Agent` request…
timrogers Jul 2, 2022
75465e1
Rename `userAgent` constant to `defaultUserAgent`
timrogers Jul 3, 2022
8495ae5
Export current package version as `CurrentVersion` constant
timrogers Jul 3, 2022
f65d26e
Drop PackageVersion field from Client struct
timrogers Jul 3, 2022
c963102
Rename CurrentVersion to Version
timrogers Jul 4, 2022
7279ad7
Add reminder to bump version to CONTRIBUTING.md
timrogers Jul 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions github/github.go
View file
Open in desktop
Expand Up @@ -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.


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.


headerRateLimit = "X-RateLimit-Limit"
headerRateRemaining = "X-RateLimit-Remaining"
Expand Down Expand Up @@ -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.


rateMu sync.Mutex
rateLimits [categories]Rate // Rate limits for the client as determined by the most recent API calls.

Expand Down Expand Up @@ -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.

c.common.client = c
c.Actions = (*ActionsService)(&c.common)
c.Activity = (*ActivityService)(&c.common)
Expand Down
8 changes: 7 additions & 1 deletion github/github_test.go
View file
Open in desktop
Expand Up @@ -507,10 +507,16 @@ func TestNewRequest(t *testing.T) {
t.Errorf("NewRequest(%q) Body is %v, want %v", inBody, got, want)
}

userAgent := req.Header.Get("User-Agent")

// test that default user-agent is attached to the request
if got, want := req.Header.Get("User-Agent"), c.UserAgent; got != want {
if got, want := userAgent, c.UserAgent; got != want {
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.

t.Errorf("NewRequest() User-Agent should contain %v, found %v", c.PackageVersion, userAgent)
}
}

func TestNewRequest_invalidJSON(t *testing.T) {
Expand Down