Add support for context package. #529

dmitshur · 2017-01-22T22:40:31Z

DO NOT MERGE. I plan to propose changes before we merge. Edit: Changes proposed in #526 (comment) and applied to this PR.

Resolves #526.

kevinburke · 2017-01-23T04:01:14Z

github/github.go

+//       to pass during the transition period. Once all endpoints are updated
+//       to accept ctx parameter, the old Do method can be removed, and this
+//       new do method with ctx parameter should be made to take its place.
+func (c *Client) do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) {
 	rateLimitCategory := category(req.URL.Path)


would a nil check make sense here?

I think it's unnecessary.

The context package is already documented to say that nil value of context.Context shouldn't be passed:

Do not pass a nil Context, even if a function permits it. Pass context.TODO if you are unsure about which Context to use.

So all valid code will always have non-nil value of ctx.

Invalid code can be easily caught by static analysis tools such as go vet, as the context package docs allude:

Programs that use Contexts should follow these rules to keep interfaces consistent across packages and enable static analysis tools to check context propagation

TODO is recognized by static analysis tools that determine whether Contexts are propagated correctly in a program.

kevinburke · 2017-01-23T04:01:26Z

.travis.yml

-  - 1.6.3
-  - 1.7
-  - tip
+  - 1.7.x


should we add 1.8.x here?

We should, once it comes out.

I'll update the PR at that time to include it.

Now that 1.8 is out, this is done in 1ebf55f (see line below).

kevinburke · 2017-01-23T04:03:51Z

github/github.go

 	rateLimitCategory := category(req.URL.Path)

 	// If we've hit rate limit, don't make further requests before Reset time.
 	if err := c.checkRateLimitBeforeDo(req, rateLimitCategory); err != nil {
 		return nil, err
 	}

-	resp, err := c.client.Do(req)
+	resp, err := c.client.Do(req.WithContext(ctx))


the WithContext docs state that "the provided ctx must be non-nil." worth erroring about here, or is the panic in net/http okay?

kevinburke

a

gmlewis · 2017-02-01T17:38:01Z

I like this approach, @shurcooL.
Thank you for doing this!

kevinburke · 2017-02-08T05:58:17Z

Ok this looks good to me

willnorris · 2017-02-08T21:01:09Z

github/issues_test.go

@@ -149,7 +150,7 @@ func TestIssuesService_Get(t *testing.T) {
 		fmt.Fprint(w, `{"number":1, "labels": [{"url": "u", "name": "n", "color": "c"}]}`)
 	})

-	issue, _, err := client.Issues.Get("o", "r", 1)
+	issue, _, err := client.Issues.Get(context.Background(), "o", "r", 1)


just for brevity, we might want to have a package-level:

var ctx = context.Background()

(or is that discouraged somewhere?)

I personally liked context.Background() everywhere (even in tests), but it was mostly because I wasn't sure if reusing the same context value in multiple places was okay. It turns out it is (/cc @rakyll), because of how it's implemented.

I still kinda like/don't mind context.Background() everywhere because it's readable and I don't think saving a bit of typing is worth it, but I'm also okay with with var bg = context.Background() or similar (in tests only).

Previous discussions:

Add context support, ErrUnauthorized and Doer interface for http.Client tambet/go-asana#4 (comment)

https://twitter.com/rogpeppe/status/798970167338422272 (/cc @rakyll)

dmitshur · 2017-02-18T03:49:28Z

Okay, now that 1.8 is out, this is unblocked.

I wanted to make some progress here. I decided to prototype the rest of the change. Basically, change all the endpoints to accept context.Context and finish this PR. The idea is to see if doing that provides any additional insights. With some assistance from tools, doing it wasn't too hard.

Also, doing a global replace of context.Background() in tests with bg, if we decide it's worth doing, is trivial. For now, I just used context.Background() because it was easier for me.

I'll push the final version and post my observations afterwards.

dmitshur · 2017-02-18T04:05:15Z

Okay, here are my 3 observations from prototyping the entire PR:

When I started updating the entire API, I noticed some of the endpoints I had to update were deprecated. For example, RateLimit is deprecated in favor of RateLimits, RepositoriesService.ListServiceHooks is deprecated in favor of Client.ListServiceHooks, etc.

Sure, I could just update them too, but I figured, since their API will change anyway... and this is going to be a huge breaking API change, why not take the opportunity to also delete the deprecated methods and such. If you're going to change your APIs, you might as well stop using deprecated methods; it shouldn't be much extra effort.

Thoughts on that? Is it a good idea to remove the deprecated stuff as part of this PR, or should I just update their APIs and keep them?

Additionally, I kinda went to an extreme and removed all deprecated things, even ones that wouldn't be affected by API changes. Please see the entire commit e11b461 and review it carefully, and let me know your thoughts. Do we delete only the affected deprecated APIs? All of them (as currently is)? Or don't remove any?
In support the context package #526 (comment), I outlined some of the options for new API. One of the open questions was whether or not Do method should get context.Context as first argument or not. It's absolutely possible to go either way, because req *http.Request is the next parameter, and it's always possible to pass the context via the request.

I currently went with Do(ctx context.Context, req *http.Request, v interface{}) but after doing the entire PR, I'm starting to get second thoughts.

Basically, it feels weird that there are 2 ways to pass the context. It feels especially weird when you don't want to pass it, as in the case of some tests. Leaving signature of Do as Do(req *http.Request, v interface{}) would make it a lot easier not to set a context.

Consider this code from some of the tests:
```
req, _ := client.NewRequest("GET", "/", nil)
resp, err := client.Do(context.Background(), req, nil)
```
If we simply passed the context to Do via the request, it becomes much more natural not to set the context:
```
req, _ := client.NewRequest("GET", "/", nil)
resp, err := client.Do(req, nil)
```
Or set it explicitly:
```
req, _ := client.NewRequest("GET", "/", nil)
resp, err := client.Do(req.WithContext(ctx), nil)
```
This is a huge PR with many files, lines touched. Is there anything I can do to make it easier to review? It's already split into separate logical commits, but I'm talking about the large commit that adds context to all endpoints.

Any ideas for making it more review friendly? @gmlewis?

I'm going to let this sit as is and think about point 2 myself. If I still think it's a good idea, I might try to change signature of Do and see how well it feels.

dmitshur · 2017-02-18T04:48:38Z

I've thought about observation 2 some more, and decided to make Do take not take separate context.Context parameter after all.

See commit 1563515, the rationale is inside its commit message:

Pass context to Do via request, instead of as a separate parameter.
Do(req *http.Request, v interface{})
It seems cleaner because it eliminates the question of where to put
context. With the previous version that took 3 parameters, it was
possible (in theory) to pass the context via request or via first
parameter. Now, that ambiguity is gone, there's only one way.

It should be most helpful in situations where you have a request
that was made elsewhere, and you want to preserve its original context,
so you don't have to do something as awkward as:
ghClient.Do(req.Context(), req, ...)
Instead, it's simply:
ghClient.Do(req, ...)

gmlewis

I'm going to stop the review at this point and see if you agree with splitting this into 3 PRs.
Thank you, @shurcooL!

gmlewis · 2017-02-18T15:17:24Z

examples/basicauth/main.go


 	// Is this a two-factor auth error? If so, prompt for OTP and try again.
 	if _, ok := err.(*github.TwoFactorAuthError); err != nil && ok {
 		fmt.Print("\nGitHub OTP: ")
 		otp, _ := r.ReadString('\n')
 		tp.OTP = strings.TrimSpace(otp)
-		user, _, err = client.Users.Get("")
+		user, _, err = client.Users.Get(context.Background(), "")


I don't think we want to show calling context.Background twice in a row within a single function as an example to be followed. Instead, please assign ctx := context.Background() once before or after line 37 and then use ctx in both locations in place of context.Background().

In examples I think it would be better to use context.TODO() tbh to emphasize to the user that they should set as appropriate.

Hmmm... I'm not sure I agree with that.

Most clients that are not being used within a server will simply want ctx := context.Background() and as such is a fine example, especially for newbie Go devs. More experienced Go programmers will realize that they can replace the context with any one that they already have in operation and will have no problem swapping it out for their own.

But having said that, I don't feel really strongly about TODO vs Background.

I feel more strongly, though, about not duplicating the call and instead assigning to a ctx variable, as that is more idiomatic Go.

Yeah, using context.TODO() can be seen in two ways:

It's a TODO for the reader to replace it with a proper context.

It's a TODO that we, the library authors, haven't resolved.

I think you want it to be seen as 1, but it might be seen as 2... So I'd rather use context.Background() over context.TODO() for that reason. I think it's clear that someone can/should replace context.Background() with their own context if they need/want to.

@gmlewis: I feel more strongly, though, about not duplicating the call and instead assigning to a ctx variable, as that is more idiomatic Go.

Done in ed4cdce, PTAL.

gmlewis · 2017-02-18T15:39:54Z

github/event_types.go

@@ -130,19 +130,6 @@ type GollumEvent struct {
 	Installation *Installation `json:"installation,omitempty"`
 }

-// IssueActivityEvent represents the payload delivered by Issue webhook.


I read your discussion about deprecation and agree, but now that I look at this, I think we want 3 separate PRs to make for a clean and easy-to-understand history:

PR#1 - Update repo to Go 1.7 and Go 1.8 and bump version number

PR#2 - Remove everything that is deprecated

PR#3 - Support context.Context (only)

If that order is not correct, then put them in the correct order.
My point here is that PR#2 and PR#3 are completely orthogonal, or should be made so as much as possible.

Thoughts?

Sounds great to me. I'll break it up into 3 PRs, it's just a matter of splitting up those commits, I won't have to do anything extra. :)

However, the tests of 3rd PR will fail until 1st is merged.

However, the tests of 3rd PR will fail until 1st is merged.

Actually, I just came up with a really sneaky way of getting around that. 😜

kevinburke

If there are any sorts of tools that can detect unused function parameters, those might be useful for detecting (in an automated way) a missing req.WithContext(ctx).

kevinburke · 2017-02-18T17:19:16Z

examples/basicauth/main.go


 	// Is this a two-factor auth error? If so, prompt for OTP and try again.
 	if _, ok := err.(*github.TwoFactorAuthError); err != nil && ok {
 		fmt.Print("\nGitHub OTP: ")
 		otp, _ := r.ReadString('\n')
 		tp.OTP = strings.TrimSpace(otp)
-		user, _, err = client.Users.Get("")
+		user, _, err = client.Users.Get(context.Background(), "")


In examples I think it would be better to use context.TODO() tbh to emphasize to the user that they should set as appropriate.

kevinburke · 2017-02-18T17:26:46Z

Pass context to Do via request, instead of as a separate parameter.

Agree on this

dmitshur · 2017-02-18T19:18:47Z

DO NOT MERGE: Please note the base of this PR is has been changed to remove-deprecated-apis branch rather than master. This PR is for review purposes only. Once #555 is merged, I will change base of this PR to master and it can be mergeable then.

Edit: Both #554 and #555 have been reviewed and merged to master. Now, this PR is rebased and set with base branch as master once again.

This is part 3 of 3 PRs to resolve #526 (as was suggested to split this PR in #529 (comment)).

This is a breaking API change. Prototype the change on one endpoint: IssuesService.Get(ctx context.Context, owner string, repo string, number int) Note that the new do method is temporary, needed only so that I don't have to change all other endpoints at the same time (otherwise they would have build errors).

The idea is to try to change everything else, and see if any additional insight comes from doing that.

@dominikh

These two methods make custom HTTP calls and do not use the github.Client.Do method, and were missed. I caught them now thanks to a tool written by @dominikh that reports unused method parameters.

dmitshur · 2017-02-20T21:00:58Z

Everything still looks good, I'm proceeding with the plan to merge this PR now.

Update for context support added in google/go-github#529.

This reverts commit 23d6cb9.

ctx is a common and well understood variable name that represents the current context.Context value. This is a followup to #529.

ztec · 2017-02-27T17:57:31Z

Hello,

I'm a relatively new go dev but this PR just broke completely the API of this library (as said in the description of 23d6cb9) .

I'm not able (too noob in go) to discus choices made with this context added in all Method of the API, and not willing to discus about that.
In many language we have vendor versioning with vendor locking mechanism to avoid breaking change breaking things.

I did not see any branch v1/v2 in the repository to decide which version (with or without context) I would like to use.
I'm not aware of any integrated tool in go ecosystem to fix the exact version/SHA1 go get will get.
I'm new to go and willing to know how to solve this "vendor" issue

What is your solution for people that do not want to fix all already existing codebase to add a mysterious "context" (for me) in all Method call (even if the compiler yell clearly about whats wrong) ?

Regards,
Zed

willnorris · 2017-02-27T18:21:14Z

@ztec: take a look at https://golang.org/wiki/PackageManagementTools

ztec · 2017-02-28T10:33:22Z

@willnorris So you are saying that not-so-go/unofficial/unfinished/bogous tools should be used to do vendor locking ? I find it weird because almost everything in go is "standard" and integrated in the go environment.

I'v already found libraries doing basic versioning for major breaking change like the one of this PR. They use different branches (v1/v2/...) and often state that older versions are not maintained anymore. This allow updates for older versions as well as usage by existing code.
This is fully compatible with standard tools in go.

What about your (this repo managers) responsibility about defining versions with clear information about breaking changes of the library ? Like Semver specify it for example. Or simply by creating a branch for older version with warning about maintenance ?

ps: I find tools of your link good ideas and solutions.

willnorris · 2017-02-28T16:24:55Z

I'm not trying to tell anyone what they should do, I was merely answering your question about how to handle vendoring in Go. As you probably saw on that page, there is an official tool in the works, but it's not ready yet.

The other option is to manually snapshot specific versions of your dependencies and check them in to your version control system, which is basically what many people did before the availability of the above tools. It's a lot more work, but addresses your concern about relying on unofficial or unfinished tools.

There has been discussion of versioning in general as well as tagging our releases in git (see #376). At the time, we opted not to do git tags and to increment the libraryVersion const on breaking changes. Admittedly, that doesn't really help with build breakages, since you can't read the const at that point anyway.

Managing dependency versions has been a problem within the Go community for a long time. In the time since #376, there has been much more effort to organize a "real" solution, and I'm happy to make whatever changes we need to work in that world. I'm happy to continue this discussion over on #376, so we can keep it all together.

ztec · 2017-02-28T18:04:28Z

I did not made my "user" job completely as I did not search for discussion about versioning and did not found #376. Sorry.

Thanks for your great responses.
I will follow #376 from now, hoping actions can be done to help users of this library.

Adds support for changes in go-github package in: google/go-github#529 Fixes #12.

* Update github package to support updated go-github library Adds support for changes in go-github package in: google/go-github#529 Fixes #12. * Pass gin context object instead of creating new

This is a large breaking API change. It is unavoidable and necessary to resolve google#526. This breaking change is part of bump to libraryVersion 3. It adds ctx context.Context as first parameter to all endpoint methods, including Do. Updating for this API change should be easy and the compiler will help catch instances that need to be updated. For example: main.go:193: not enough arguments in call to gh.Activity.MarkRepositoryNotificationsRead have (string, string, time.Time) want (context.Context, string, string, time.Time) ... You should pass the available context as first parameter. If your code does not have context.Context available and you want to maintain previous normal behavior, use context.Background(). Don't pass nil context; use context.TODO() instead if you wish to delay figuring out the best context to use. Then you can address the TODO at a later time. Refer to documentation of package context at https://godoc.org/context for more information. This commit also changes ./tests/fields to use context.Background() instead of deprecated oauth2.NoContext. Resolves google#526.

dmitshur mentioned this pull request Jan 22, 2017

support the context package #526

Closed

kevinburke reviewed Jan 23, 2017

View reviewed changes

dmitshur added the waiting for reply label Feb 3, 2017

willnorris reviewed Feb 8, 2017

View reviewed changes

dmitshur removed the waiting for reply label Feb 18, 2017

dmitshur force-pushed the context branch from a1ec4f4 to 5047df1 Compare February 18, 2017 03:50

dmitshur changed the title ~~WIP: Add support for context package.~~ Add support for context package. Feb 18, 2017

gmlewis requested changes Feb 18, 2017

View reviewed changes

kevinburke reviewed Feb 18, 2017

View reviewed changes

This was referenced Feb 18, 2017

Require Go 1.7 and newer. #554

Merged

Remove deprecated identifiers. #555

Merged

dmitshur force-pushed the context branch from 1563515 to 289d63c Compare February 18, 2017 19:16

dmitshur changed the base branch from master to remove-deprecated-apis February 18, 2017 19:17

dmitshur force-pushed the remove-deprecated-apis branch from 66f3140 to dae2143 Compare February 18, 2017 19:19

dmitshur force-pushed the context branch from 289d63c to bce078e Compare February 18, 2017 19:20

dmitshur force-pushed the remove-deprecated-apis branch from dae2143 to f34b33d Compare February 18, 2017 19:29

dmitshur changed the base branch from remove-deprecated-apis to master February 18, 2017 19:32

dmitshur added 3 commits February 18, 2017 14:33

Remove temporary do method copy.

3e016f5

Prototype the rest.

98e27f4

The idea is to try to change everything else, and see if any additional insight comes from doing that.

Fix GetArchiveLink, DownloadReleaseAsset methods not propagating ctx.

052be22

These two methods make custom HTTP calls and do not use the github.Client.Do method, and were missed. I caught them now thanks to a tool written by @dominikh that reports unused method parameters.

dmitshur self-assigned this Feb 20, 2017

dmitshur merged commit 23d6cb9 into master Feb 20, 2017

dmitshur deleted the context branch February 20, 2017 21:09

dmitshur added a commit to shurcooL/notifications that referenced this pull request Feb 20, 2017

githubapi: Update for go-github API change. (#1)

6d5343e

Update for context support added in google/go-github#529.

dmitshur added a commit to shurcooL/home that referenced this pull request Feb 20, 2017

Update for go-github API change. (#8)

497b76c

Update for context support added in google/go-github#529.

dghubble mentioned this pull request Feb 21, 2017

Fix Github GetUsers API call to include context dghubble/gologin#18

Merged

hkurokawa mentioned this pull request Feb 21, 2017

Cannot 'go get' due to some API changes in go-github reviewdog/reviewdog#80

Closed

muesli added a commit to muesli/go-github that referenced this pull request Feb 21, 2017

Revert "Add support for context package to all endpoints. (google#529)"

36cb4c5

This reverts commit 23d6cb9.

zcahana mentioned this pull request Feb 21, 2017

Pass a context object into github client calls istio/test-infra#71

Closed

dmitshur mentioned this pull request Feb 22, 2017

Update API inside documentation. #561

Closed

dmitshur added a commit that referenced this pull request Feb 22, 2017

Update documentation with new context parameter.

4a23999

ctx is a common and well understood variable name that represents the current context.Context value. This is a followup to #529.

willnorris mentioned this pull request Feb 28, 2017

versioning and tagging releases #376

Closed

dmitshur mentioned this pull request Mar 5, 2017

Cant get it running karanjthakkar/showmyprs.com#12

Closed

gmlewis mentioned this pull request Mar 5, 2017

Using go-github on Google App Engine with recent context changes - any interest? #578

Closed

karanjthakkar added a commit to karanjthakkar/showmyprs.com that referenced this pull request Mar 5, 2017

Update github package to support updated go-github library

b0c865f

Adds support for changes in go-github package in: google/go-github#529 Fixes #12.

karanjthakkar mentioned this pull request Mar 5, 2017

Update github package to support updated go-github library karanjthakkar/showmyprs.com#14

Merged

alindeman mentioned this pull request Mar 6, 2017

Adds support for context.Context interagent/schematic#41

Merged

tianon mentioned this pull request Apr 7, 2017

Update to Go 1.8 and add a program-global "context" object yosifkit/official-images-issue-labeler#10

Merged

dmitshur mentioned this pull request Feb 5, 2018

Add Context bradleyfalzon/ghinstallation#8

Closed

nzoschke mentioned this pull request Mar 9, 2018

Context chrismalek/oktasdk-go#4

Open

dmitshur mentioned this pull request May 9, 2018

Propagate context on token refresh requests bradleyfalzon/ghinstallation#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for context package. #529

Add support for context package. #529

dmitshur commented Jan 22, 2017 •

edited

Loading

kevinburke Jan 23, 2017

dmitshur Jan 23, 2017 •

edited

Loading

kevinburke Jan 23, 2017

dmitshur Jan 23, 2017 •

edited

Loading

dmitshur Feb 18, 2017

kevinburke Feb 18, 2017

kevinburke Jan 23, 2017

kevinburke left a comment

gmlewis commented Feb 1, 2017

kevinburke commented Feb 8, 2017

willnorris Feb 8, 2017

dmitshur Feb 8, 2017

dmitshur commented Feb 18, 2017

dmitshur commented Feb 18, 2017 •

edited

Loading

dmitshur commented Feb 18, 2017 •

edited

Loading

gmlewis left a comment

gmlewis Feb 18, 2017

kevinburke Feb 18, 2017

gmlewis Feb 18, 2017

dmitshur Feb 18, 2017 •

edited

Loading

dmitshur Feb 18, 2017

gmlewis Feb 18, 2017

dmitshur Feb 18, 2017

dmitshur Feb 18, 2017

kevinburke left a comment

kevinburke Feb 18, 2017

kevinburke commented Feb 18, 2017

dmitshur commented Feb 18, 2017 •

edited

Loading

dmitshur commented Feb 20, 2017

ztec commented Feb 27, 2017

willnorris commented Feb 27, 2017 •

edited

Loading

ztec commented Feb 28, 2017 •

edited

Loading

willnorris commented Feb 28, 2017

ztec commented Feb 28, 2017

Add support for context package. #529

Add support for context package. #529

Conversation

dmitshur commented Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

dmitshur Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinburke left a comment

Choose a reason for hiding this comment

gmlewis commented Feb 1, 2017

kevinburke commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur commented Feb 18, 2017

dmitshur commented Feb 18, 2017 • edited Loading

dmitshur commented Feb 18, 2017 • edited Loading

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitshur Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinburke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinburke commented Feb 18, 2017

dmitshur commented Feb 18, 2017 • edited Loading

dmitshur commented Feb 20, 2017

ztec commented Feb 27, 2017

willnorris commented Feb 27, 2017 • edited Loading

ztec commented Feb 28, 2017 • edited Loading

willnorris commented Feb 28, 2017

ztec commented Feb 28, 2017

dmitshur commented Jan 22, 2017 •

edited

Loading

dmitshur Jan 23, 2017 •

edited

Loading

dmitshur Jan 23, 2017 •

edited

Loading

dmitshur commented Feb 18, 2017 •

edited

Loading

dmitshur commented Feb 18, 2017 •

edited

Loading

dmitshur Feb 18, 2017 •

edited

Loading

dmitshur commented Feb 18, 2017 •

edited

Loading

willnorris commented Feb 27, 2017 •

edited

Loading

ztec commented Feb 28, 2017 •

edited

Loading