Add golang context support

[kellyma2]

This is a large cross cutting change to the existing interfaces for go-gerrit that introduces context support to all interfaces drilling down to the HTTP level.

Contexts enable support for both passing information through (eg: tracing, logging, etc) and features like cancellation of slow operations (eg: REST calls).

In order for consumers of this breaking change to adapt their code to use the new interface they must add a context parameter to their invocations. In most cases this can just be context.Background()

Fixes #99.

[kellyma2]

@andygrunwald

[kellyma2]

@andygrunwald bump

[andygrunwald]

@kellyma2 Thanks a lot for this PR. The change itself is looking good.

This change would be a breaking change in the library.
I think this breaking change is for the better for the library. An alternative would be to duplicate all function calls and add a suffix like ...WithContext(...).

This is an option, but I hope that all downstream users have go modules active.

Before merging, I am highlighting a few folks that I know using this library to get their objections (because in the end, they need to make the change in their applications):

@opalmer @dmitshur @banksean @chyroc @wwade @hanwen @lukegb @pastel001 @afq984 Any objections from your end?

If there is no objections until Wednesday, 1st of November 2023, 8 am UTC, I will merge it.

[kellyma2]

Obviously I have my personal opinions on merit, but I like the breaking change as it's a clear signal (and very easy to fix in consumer code). Alternatively, a versioned API could work, but this also represents a fork in the road.

[hanwen]

I don't run go-gerrit code myself, but know folks that do.

I have a dissenting opinion.

Merging this change will gratuitiously break users that might want to upgrade to get a bugfix or more Gerrit API coverage, without providing any upside. Why not add the WithContext flavors of the methods? Or alternatively,

type AccountsService struct {
  ctx context.Context
  client *http.Client
}
 
func (a *AccountService) NewWithContext(ctx context.Context) *AccountService {
  return &AccountService{ctx, a.client}
}

See also https://go.dev/blog/context-and-structs.

[kellyma2]

I don't run go-gerrit code myself, but know folks that do.

I have a dissenting opinion.

Merging this change will gratuitiously break users that might want to upgrade to get a bugfix or more Gerrit API coverage, without providing any upside. Why not add the WithContext flavors of the methods? Or alternatively,

type AccountsService struct {
  ctx context.Context
  client *http.Client
}
 
func (a *AccountService) NewWithContext(ctx context.Context) *AccountService {
  return &AccountService{ctx, a.client}
}

See also https://go.dev/blog/context-and-structs.

@hanwen Looking at the size of the change, adding WithContext methods every does noticably increase the library surface area. In your view, once introduced does every method have to maintain a context-less and a WithContext variant? For new methods do both need to be provided?

In the net/http example, the lifecycle of a single request is fairly narrow. This will tend to lend itself to the context-in-struct approach, whereas an API where the instantiated structs have a longer life (ie: more than a single use) would be less ideal for this. What's the expectation with the individual struct instances in the case of go-gerrit?

The third alternative (which I mentioned) is to do something like what was done with go-redis. They have a versioned API and for v8 they dropped the non-context versions and replaced them with the context-using versions. This has the same problem you're describing though. However, the breakage seems to have been ok in that case and I assume that go-redis has pretty broad usage.

Fwiw, I'm ok with re-working this to be like either option (1) or option (3), but my gut feeling is that option (2) would be confusing for users of this library.

edit: Should add that another option is to keep a branch with a pre-break release tag and just port bugfixes over if desired. Not sure how desirable that is for @andygrunwald but at least folks can get bugfixes if they want them?

[hanwen]

What's the expectation with the individual struct instances in the case of go-gerrit?

if users care about context, they should create a new one per context.

However, the breakage seems to have been ok

I doubt that someone needing to upgrade go-redis in a large company monorepo would have been amused with the move.

[kellyma2]

What's the expectation with the individual struct instances in the case of go-gerrit?

if users care about context, they should create a new one per context.

In the net/http case, this is actually implied by how the API works, in the case of go-gerrit the API doesn't have the same request model and thus doesn't exactly imply this. Can users be forced to do this? Yes. Is it a natural use of the given API? I'd suggest no. :)

However, the breakage seems to have been ok

I doubt that someone needing to upgrade go-redis in a large company monorepo would have been amused with the move.

YMMV, but since the fix is insert context.TODO() or context.Background() as the first parameter to all of the API calls, this seems pretty mechanical, but I can totally see how this is a matter of opinion/perspective. There has to be some consideration of the burden on the library maintainer(s) as well though, which is why I'm curious as to your opinion of what the policy on the xxxxWithContext() approach would be.

[hanwen]

to all of the API calls, this seems pretty mechanical, but I can totally see how this is a matter of opinion/perspective.

I'm saying this out of experience with google monorepo, where -due to its size- changing a wide swath of code is actually a lot of work. The Go team is extremely conservative with introducing incompatible changes, and I think it is a good example to follow.

There has to be some consideration of the burden on the library maintainer(s) as well though, which is why I'm curious as to your opinion of what the policy on the xxxxWithContext() approach would be.

I don't mind adding XxxWithContext(). I am arguing against removing the Xxx (without WithContext) methods. The old methods can trivially ("mechanically") implemented in terms of XxxWithContext, by just supplying a context.Background().

Full-disclosure: I don't work on Gerrit anymore, so I don't care very much which way this goes, but Andy asked my opinion, so I am giving it.

[kellyma2]

There has to be some consideration of the burden on the library maintainer(s) as well though, which is why I'm curious as to your opinion of what the policy on the xxxxWithContext() approach would be.

I don't mind adding XxxWithContext(). I am arguing against removing the Xxx (without WithContext) methods. The old methods can trivially ("mechanically") implemented in terms of XxxWithContext, by just supplying a context.Background().

Sorry, just wanted to clarify what I think you're suggesting here. It sounds like to me you're suggesting that (1) we add xxxWithContext() for existing methods, (2) we add both xxxWithContext() and xxx() for new methods?

[hanwen]

yes for (1). At your option (2) as well, but it's less important for backward compatibility.

[afq984]

Just my two cents, assuming that we don't have breaking changes too often, I'd rather downstreams figure out a way to pay an one-off migration cost, instead of keeping the extra maintenance cost in the upstream (and duplicated API, docs for everyone) indefinitely, as I think it's much harder to get people to volunteer on open source work

Especially since #153 (comment) indicates that the changes are mechanical and automatable.

Well, maybe there's some way to allow downstreams to migrate call sites incrementally instead of all at once, then it'd be an easier breaking change to take on. But I don't have an idea how to make it.

[kellyma2]

@andygrunwald care to weigh in on your preferences here?

[andygrunwald]

Thanks @hanwen @afq984 @kellyma2, for your voice. I appreciate this.
We agree that for smaller codebases, those changes might be doable quickly.
I can also imagine that for larger usage (like Google), this change might be more difficult to make.

I am aware that this library is used inside the google mono repo.
I am inclined to merge this breaking change, mainly due to @afq984 arguments. From the history of this library, there hasn't been much breaking change (if any). At the moment, I don't foresee more.

A question to the google folks (@afq984 @dmitshur @banksean @lukegb && @hanwen):
Are any of you still working on the Gerrit code inside your mono repo, and can you initiate the change/upgrade once this PR is merged?
If not, is there a possibility that one of you will reach out internally to folks who work on the parts of the codebase?
An indicator would help here.

[lukegb]

On the Google side I'm happy to chase the monorepo users, since I think there's relatively few impacted callsites there. There are a few uses in Chrome infrastructure as well I think but they'll probably notice when they try to update and it should be pretty obvious what to do...

[andygrunwald]

Great. Thanks @lukegb. I am moving on and merging this.
Thanks to @kellyma2 for the PR in the first place. Thanks for everyone for your thoughts and involvement.