-
Notifications
You must be signed in to change notification settings - Fork 188
Added API versioning support #379
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
Conversation
|
@microsoft-github-policy-service agree company="City National Bank" |
@microsoft-github-policy-service agree company="City National Bank" |
HowardWolosky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, @variableresistor! I also appreciate you calling attention to the fact that GitHub is introducing API versioning...I completely missed that announcement.
I love the idea of making the module API version aware, but I currently disagree with the methodology that you're using to introduce it. We're likely to adopt newer API versions on a per-API basis, one at a time. That means we wouldn't want a module-wide mechanism for changing the API Version, but rather the ability to change it on every REST call.
HowardWolosky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I had published this review earlier, but it still shows as pending.
Co-authored-by: Howard Wolosky <HowardWolosky@users.noreply.github.com>
HowardWolosky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor nits, and then should be good to merge.
Co-authored-by: Howard Wolosky <HowardWolosky@users.noreply.github.com>
Co-authored-by: Howard Wolosky <HowardWolosky@users.noreply.github.com>
Co-authored-by: Howard Wolosky <HowardWolosky@users.noreply.github.com>
|
/azp run PowerShellForGitHub-CI |
|
Azure Pipelines successfully started running 1 pipeline(s). |
HowardWolosky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now. Thanks for the contribution and for noticing this pending change in the first place!
Tests are sufficiently passing (pretty much all of the failing tests are the Projects tests which we know will fail (due to #380) -- they're about to be disabled).
Description
GitHub is introducing versioned API's. In order to get the expected behavior of an API, you need to invoke the API with its version in the request header. This change updates the core functions (
Invoke-GHRestMethodandInvoke-GHRestMethodMultipleResult) to accept an ApiVersion as part of their param sets so that as the module continues to add new functionality, methods can request the appropriate API Version for their individual calls.Features Added
Fixes #377
References
To infinity and beyond: enabling the future of GitHub’s REST API with API versioning
API Versions
Checklist
- [ ] Formatters were created for any new types being added.