Proposal: Make preview features configurable

[gauntface]

Problem

A project I'm working on has an issue where a list of repos is returning a Server Error due to an additional Accept header being added (The "mediaTypeCodesOfConductPreview" header).

Removing this header fixes the problem but I can't do this with the current library implementation.

Solution

Instead of adding these headers by default, we could move them to an Options parameter similar to ListOptions.

type RequestOptions {
  acceptHeaders []string
}

Pros

• Allows developers to control which headers to add (including headers the library may not control)
• Removes the maintenance of adding / removing headers from library owners
• Opens the door for any additional request options in the future.
• This will remove the need for comments like // TODO: remove custom Accept headers when APIs fully launch.

Cons

• Forces users to care about headers
  • While a con, users should know if a feature they are using is preview or not and subject to change

Concerns

The only major concern I have with this is approach is that it gets fuzzy what to do with entirely new API's (for example the checks API).

For consistency the API could still require the developer to add the header. For a sane API the API could add the header with the expectation that the header is removed as soon as possible, but again moves the burden on to the library.

[gmlewis]

First, I wonder why GitHub would return a Server Error for adding a documented preview header? Should you contact support@github.com to ask why that is happening?

Second, another option might be to provide a method to globally turn off (or back on) the addition of preview headers within this package... then the API remains the same for those unaffected by preview headers, but a simple switch could shut them off entirely (for one or more endpoint calls).

[gauntface]

Yes to reaching out to GitHub support.

The global switch would unblock us, but would block others from adding different headers (i.e. headers the library doesn't have built in).

[gauntface]

We could do the following:

A method that configures the default option to add or not add the headers and then use the RequestOptions as a way to override the defaults regardless.

This allows both a global configuration for default behavior with an easy way to override this regardless.

Another option is to simply keep everything as is and use RequestOptions as a way to override the defaults.

[gmlewis]

I rather like the option where the default behavior is to use all preview custom headers (as has been the case with this package throughout its history), then be able to override this behavior for special cases if necessary.

[gauntface]

Out of interest, what was the reasoning behind using the preview headers by default?

[gmlewis]

I believe @willnorris made the decision at the start to attempt to stay current with all the preview features developed by the GitHub Developer team. We chatted about this and if I recall correctly, one of his reasons was that it was a great way to let users try out features early and give feedback to the GitHub team as to what works, what doesn't, and what could be improved.

In our CONTRIBUTING.md file, Will wrote:

But because go-github is a client library for the GitHub API, which itself changes behavior, and because we are typically pretty aggressive about implementing preview features of the GitHub API, we've adopted the following versioning policy...

[willnorris]

I'll have to give some more thought to how I feel about changing the default behavior, but a few additional notes:

• What @gmlewis said matches my recollection of why we enabled this by default... the preview periods for new features were often quite long, and we (as well as a number of other users) were wanting to use them, so it was simplest to just always enable them. And prior to this bug, we've never had a reported instance of this causing a problem.

• client.NewRequest and client.Do are very intentionally exported methods, rather then keeping them private to the github package. Much of the interesting http logic lives in these two methods, and all of the endpoint specific methods that most people use are intended to be thin wrappers around them (construct URL and request, send request, minimally handle response, return everything). The intent has always been that if you needed to modify how the request was constructed, you should be able to simply duplicate that method with whatever custom changes you needed. Ideally, they'd be thin enough wrappers that you wouldn't be duplicating much code. We can discuss whether those wrappers are still sufficiently thin for that to be a reasonable approach.

• A slight alternative to @gauntface's suggestion could be to add an AcceptHeaders field onto the options structs (which I'm still not completley wild about), but if it is nil, then use a default set of headers which includes the relevant preview headers. It would be completely compatible with the behavior today... if you do nothing, then you get all the preview functionality. If you want to disable that, then you set AcceptHeaders to an empty slice (or whatever headers you do want to send).

[ahmetb]

I've also reached out to GitHub support, they said they've forwarded it to their engineering team. I also opened #1037 which provides a direct repro with curl –feel free to close that issue, it's very similar to this. (No opinion here on how to address this issue.)

[gauntface]

Just got hit by this again.

From @willnorris's comments:

• The intent has always been that if you needed to modify how the request was constructed, you should be able to simply duplicate that method with whatever custom changes you needed.

How do I change the behavior? I don't believe I can overwrite the Do() method which I thought I'd need for the client to call my method instead of the clients method.

Regarding the AcceptHeaders options, would you be ok with that proposal @gmlewis?

[willnorris]

I mentioned this to @gauntface in chat as well, but repeating here for others. My comment about duplicating methods was not about overwriting the client.Do method, but rather providing alternate implementations of the various library methods that call client.Do (client.Users.Get, etc).

It's not ideal to have to duplicate those, but hopefully you shouldn't have to do it very often, and the fact that client.NewRequest and client.Do are exported, you're not having to duplicate too much code.

Though as I said above, that's just the current design. I'm open to alternate approaches to make this kind of thing easier (or even rethinking our stance on preview functionality altogether).

[gmlewis]

I think I would prefer a mechanism to alter what is being sent via AcceptHeaders over providing alternate implementations of specific endpoints, only because the latter might dramatically expand our API surface (depending on how many alternates we end up making)... and subsequently muddy our auto-generated godocs.

It seems to me like altering the AcceptHeaders could even allow the manipulation of them between API calls while still keeping a similar API surface to what we currently have.

But at the end of the day, I'm not strongly for or against either method... so do you feel like putting together a PR @gauntface ? You and others could try it out and see how you like it in practice.

[willnorris]

Oh, I certainly wasn't suggesting that we would put alternate implementations into go-github. The point was that if a user of go-github ever wanted to do something different than the default behavior, they could write their own alternate implementations if they needed to. We provide the raw ingredients (go-querystring, client.NewRequest, client.Do, etc), and they can use them to make their own custom dish. 😄

That said, it certainly seems reasonable for us to provide a simple way to disable preview functionality (either wholesale for an entire client, or on a per-function basis) natively in go-github.

[gmlewis]

I'm wondering if a ~WithContext~ context.WithValue solution might be reasonable for this situation to disable preview functionality, similar to #1907 and also suggested in #1920 ?

[gauntface]

Can you share some examples of how WithContext would be used here?

[willnorris]

I'm assuming Glenn meant context.WithValue. You could tuck a value into the context that indicated the API previews you wanted to enable or disable. Then the methods that would normally attach one of the preview accept headers would check that context value first.

[willnorris]

So from the user's perspective, it would be something like:

client := github.NewClient()
ctx := context.Background()
ctx = github.EnablePreviewFeatures(ctx, github.PreviewFoo, github.PreviewBar)

// or alternately
ctx = github.DisablePreviewFeatures(ctx, github.PreviewFoo, github.PreviewBar)

client.Orgs.DoSomething(ctx, ...)

And we'd probably have some internal helper methods in the library to check for specific previews being enabled before attaching the accept header.

[gauntface]

Overall I love this idea.

Especially like that it allows go-github to have defaults it thinks are best and then allows users to alter behavior as necessary.