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.

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

Support new GitHub v3 API calendar-based versioning #2581

Merged
merged 3 commits into from Dec 9, 2022

Conversation

gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Nov 29, 2022

Fixes: #2580.

@willnorris - please see if you agree with this approach for supporting the new GitHub v3 API versioning mechanism.

My thought is that when they add breaking API changes, that we will continue to support them, but will now add the WithVersion option to each request that needs it.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Nov 29, 2022

Codecov Report

Merging #2581 (e76acfa) into master (18cd63d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head e76acfa differs from pull request most recent head eb1e95a. Consider uploading reports for the commit eb1e95a to get more accurate results

@@           Coverage Diff           @@
##           master    #2581   +/-   ##
=======================================
  Coverage   97.98%   97.98%           
=======================================
  Files         125      125           
  Lines       10875    10894   +19     
=======================================
+ Hits        10656    10675   +19     
  Misses        150      150           
  Partials       69       69           
Impacted Files Coverage Δ
github/github.go 97.45% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Copy link
Collaborator

@willnorris willnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this definitely makes sense for what the go API should look like. In practice, I think we would want per-method version overrides (at least in the core library) to be rare and temporary.

My read of the GitHub docs is that they will be revving the entire API to each new date-based version, even if only a few methods have breaking changes. Other methods will accept the new version with their existing functionality. So when a new date-based version of the GitHub API is released, I think we'll want to:

  • update each method that had breaking changes, overriding their per-method API version header. This may happen in one or multiple commits and PRs, and is all done in the main branch.
  • once all of the methods with breaking changes have been updated, have a final commit that bumps the default API version, and remove all of the per-method overrides. That would now get a major version bump when the next go-github release is made.

We'll probably want to clearly identify in the README which version of the GitHub API this version of go-github works with (in case users need a specific version). And depending on how frequently GitHub revs their date-based versions, we might eventually have a table of the last go-github version that supported each GitHub API version?

Exporting the NewRequest and Do methods has meant that go-github users could always customize their API calls instead of using the providing method wrappers. Adding RequestOption like you have here keeps that simple and easily accessible, so I really like this.

github/github.go Outdated
// RequestOption represents an option that can modify an http.Request.
type RequestOption func(req *http.Request)

// WithVersion modifies the GitHub v3 API version for this individual request.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change "modifies" to "overrides" or "replaces", just to emphasize that this completely replaces the previous version?

I could imagine other RequestOptions that add a new header without replacing existing ones (for example, if we were to have done something like this for adding the preview Accept headers)

Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@gmlewis
Copy link
Collaborator Author

gmlewis commented Nov 29, 2022

Excellent feedback, @willnorris - thank you very much!

I've attempted to capture your ideas into the README.md file.
Please take a look and see if that is what you had in mind.

Copy link
Contributor

@raynigon raynigon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gmlewis
Copy link
Collaborator Author

gmlewis commented Dec 9, 2022

Thank you, @raynigon !
Merging.

@gmlewis gmlewis merged commit 9d020d7 into google:master Dec 9, 2022
@gmlewis gmlewis deleted the support-api-versioning branch December 9, 2022 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new calendar-based versioning for the REST API
3 participants