Remove custom media type for Pull Request Reviews API #638

Closed
gmlewis opened this Issue May 11, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@gmlewis
Collaborator

gmlewis commented May 11, 2017

The custom media type application/vnd.github.black-cat-preview+json can be removed from go-github as the "Pull Request Reviews API" is no longer in preview but has become official.

See GitHub Developer announcement:
https://developer.github.com/changes/2017-05-09-end-black-cat-preview/

@varadarajana

This comment has been minimized.

Show comment
Hide comment
@varadarajana

varadarajana May 11, 2017

Contributor

@gmlewis please assign this to me

Contributor

varadarajana commented May 11, 2017

@gmlewis please assign this to me

@varadarajana

This comment has been minimized.

Show comment
Hide comment
@varadarajana

varadarajana May 12, 2017

Contributor

@gmlewis @shurcooL Pull Requests have three categories - Reviews, ReviewComments and Review Requests. Is this change required for all of them. The documentation points are reviews only.

Contributor

varadarajana commented May 12, 2017

@gmlewis @shurcooL Pull Requests have three categories - Reviews, ReviewComments and Review Requests. Is this change required for all of them. The documentation points are reviews only.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur May 12, 2017

Collaborator

I think it's all endpoints that currently set the custom application/vnd.github.black-cat-preview+json Accept header. They no longer need to.

Collaborator

dmitshur commented May 12, 2017

I think it's all endpoints that currently set the custom application/vnd.github.black-cat-preview+json Accept header. They no longer need to.

varadarajana added a commit to varadarajana/go-github that referenced this issue May 15, 2017

varadarajana added a commit to varadarajana/go-github that referenced this issue May 15, 2017

varadarajana added a commit to varadarajana/go-github that referenced this issue May 15, 2017

varadarajana added a commit to varadarajana/go-github that referenced this issue May 15, 2017

@dmitshur dmitshur closed this in 1151660 May 15, 2017

BOTBrad pushed a commit to BOTBrad/go-github that referenced this issue Jun 16, 2017

Remove custom media type for Pull Request Reviews API. (#641)
The Pull Request Reviews API (include Review Requests) has become an
official part of GitHub API v3, so the preview API media type is no
longer needed. See announcement at
https://developer.github.com/changes/2017-05-09-end-black-cat-preview/.

Resolves #638.
@swsnider

This comment has been minimized.

Show comment
Hide comment
@swsnider

swsnider Aug 10, 2017

This change breaks Enterprise users -- we don't have reviews out of preview yet :(.

This change breaks Enterprise users -- we don't have reviews out of preview yet :(.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 10, 2017

Collaborator

Thanks for reporting @swsnider.

Re-opening for further consideration.

How should we go about dealing with this, @gmlewis?

A short term solution is to revert this removal of the preview API.

However, a long term solution is deciding on our policy. Should latest master target GitHub.com
API only, or GitHub.com and GitHub Enterprise APIs? The challenge with doing the latter from what I can see is that their API chances/announcements are more difficult to track down for Enterprise API. But maybe I haven't looked hard enough.

Collaborator

dmitshur commented Aug 10, 2017

Thanks for reporting @swsnider.

Re-opening for further consideration.

How should we go about dealing with this, @gmlewis?

A short term solution is to revert this removal of the preview API.

However, a long term solution is deciding on our policy. Should latest master target GitHub.com
API only, or GitHub.com and GitHub Enterprise APIs? The challenge with doing the latter from what I can see is that their API chances/announcements are more difficult to track down for Enterprise API. But maybe I haven't looked hard enough.

@dmitshur dmitshur reopened this Aug 10, 2017

@swsnider

This comment has been minimized.

Show comment
Hide comment
@swsnider

swsnider Aug 10, 2017

The problem is that even after the Enterprise version with this fixed is released, you're typically still at the mercy of your IT org to upgrade your instance in a timely manner, so this may not be soluble by this project :(.

The problem is that even after the Enterprise version with this fixed is released, you're typically still at the mercy of your IT org to upgrade your instance in a timely manner, so this may not be soluble by this project :(.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 10, 2017

Collaborator

Right. So it might be the case that a better/more viable solution is to, unfortunately, use an older version of go-github via vendoring.

Collaborator

dmitshur commented Aug 10, 2017

Right. So it might be the case that a better/more viable solution is to, unfortunately, use an older version of go-github via vendoring.

@swsnider

This comment has been minimized.

Show comment
Hide comment
@swsnider

swsnider Aug 10, 2017

Yeah, that's what I'm doing for now

Yeah, that's what I'm doing for now

@elliott-beach

This comment has been minimized.

Show comment
Hide comment
@elliott-beach

elliott-beach Aug 20, 2017

Contributor

I think the "good-for-beginners" label could be removed from this, unless there is a way for a random person like me to solve it?

Contributor

elliott-beach commented Aug 20, 2017

I think the "good-for-beginners" label could be removed from this, unless there is a way for a random person like me to solve it?

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Aug 21, 2017

Collaborator

I had a talk with @willnorris about this, and I believe our resolution was that semantic versioning (#376) would be the best way to support GitHub Enterprise API users (and that they would need to vendor the version that best supported their use-case), and that the latest master target the latest GitHub API.

As such, I think the "good-for-beginners" label still holds for this issue.

Collaborator

gmlewis commented Aug 21, 2017

I had a talk with @willnorris about this, and I believe our resolution was that semantic versioning (#376) would be the best way to support GitHub Enterprise API users (and that they would need to vendor the version that best supported their use-case), and that the latest master target the latest GitHub API.

As such, I think the "good-for-beginners" label still holds for this issue.

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Aug 22, 2017

Member

We can't help IT departments that are slow to upgrade, and vendoring (together with semver) is the right solution there.

But I kinda like the policy of not removing preview media types until they have been released in GitHub Enterprise (assuming we can track that without too much trouble). We could go ahead and create issues like this when APIs come out of preview for github.com, and apply a special issue label, but don't actually remove the code. Then, when the next GitHub Enterprise version is released, we go through the issues to see which ones we can remove. It's a little bit of manual work but it's only done periodically.

Member

willnorris commented Aug 22, 2017

We can't help IT departments that are slow to upgrade, and vendoring (together with semver) is the right solution there.

But I kinda like the policy of not removing preview media types until they have been released in GitHub Enterprise (assuming we can track that without too much trouble). We could go ahead and create issues like this when APIs come out of preview for github.com, and apply a special issue label, but don't actually remove the code. Then, when the next GitHub Enterprise version is released, we go through the issues to see which ones we can remove. It's a little bit of manual work but it's only done periodically.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 22, 2017

Collaborator

As such, I think the "good-for-beginners" label still holds for this issue.

Note, the original task of removing the media type has already been done in 1151660, @gmlewis. I've reopened the issue only to discuss the outcome regarding Enterprise not having support for this yet. Aside from discussion, this issue isn't actionable, so I'll remove that label.

Collaborator

dmitshur commented Aug 22, 2017

As such, I think the "good-for-beginners" label still holds for this issue.

Note, the original task of removing the media type has already been done in 1151660, @gmlewis. I've reopened the issue only to discuss the outcome regarding Enterprise not having support for this yet. Aside from discussion, this issue isn't actionable, so I'll remove that label.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Aug 22, 2017

Collaborator

It also sounds like we've come to a conclusion about the long term solution. Thanks @gmlewis and @willnorris. Further discussion about semver and tagging can be resumed in #376. I'll close this issue, but feel to continue discussion there, or open a new issue if there's something specific to address.

Collaborator

dmitshur commented Aug 22, 2017

It also sounds like we've come to a conclusion about the long term solution. Thanks @gmlewis and @willnorris. Further discussion about semver and tagging can be resumed in #376. I'll close this issue, but feel to continue discussion there, or open a new issue if there's something specific to address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment