Skip to content

Invitation methods take :owner/:repo #586

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

Merged
merged 2 commits into from
Mar 11, 2017
Merged

Conversation

alindeman
Copy link
Contributor

@alindeman alindeman commented Mar 8, 2017

All repository routes on the GitHub API can accessed via /repositories/:id and /repos/:owner/:repo. For whatever reason, the documentation for the invitation routes only references the former, but both work correctly.

Repository invitations are the only functions in google/go-github to use the :id style routes, so I'd propose we change them to use :owner/:repo as that will likely be much more convenient for users of this library.

All repository routes on the GitHub API can accessed via /repositories/:id
_and_ /repos/:owner/:repo. For whatever reason, the documentation for the
invitation routes only references the former.

Repository invitations are the only functions in google/go-github to use the
:id style routes, so I'd propose we change them to use :owner/:repo as that
will likely be much more convenient for users of this library.
@dmitshur
Copy link
Member

dmitshur commented Mar 9, 2017

Hmm, this is very interesting. On the surface, what you're saying is very true. I want to dig deeper to better understand how this happened and what's going on.

These methods were implemented 9 months ago, in #373, and reviewed by @gmlewis and me.

At first, I thought this was about one of the new features we've merged in the last few days, but 9 months? I'm surprised no one has spotted this before now.

Looking at the GitHub announcement the underlying issue was about, and then the documentation, I can see how this happened. The GitHub documentation actually uses repo IDs rather than owner/repo scheme.

image

(Source: https://developer.github.com/v3/repos/invitations/#list-invitations-for-a-repository.)

As you said, both are indeed valid, but this library and most endpoints in GitHub API docs use the latter form.

I think we should contact GitHub support and ask them about this. Perhaps there's a good reason why these endpoints are documented using repo ID form rather than owner/repo. If so, we'll need to think harder about this.

Otherwise, and I suspect this is the case, it could be just an unintended outcome. If so, they might decide to fix their documentation to use the consistent owner/repo form.

If that happens, it should be much easier for us to make the decision to follow suit.

Given that this has been around for 9 months, it's a little worrisome about breaking the API... but unless I'm missing something, this alternate form must be more awkward to use anyway. Also, these endpoints are still considered as "preview API", it's not yet stable version, which gives us more room to fix things.

@alindeman
Copy link
Contributor Author

@shurcooL Here's what I got from GitHub support

As for your question -- that looks like a problem in the documentation. You should definitely be able to use the standard routes like /repos/:owner/:repo/invitations. Not sure if you remember this, but most (if not all) repository-related API endpoints can be accessed through two routes:

/repos/:owner/:repo/...

/repositories/:repo_id/...

They're equivalent and route to the same code. Not sure why the team decided to document the one with the repository ID instead of the name for this specific endpoint, but we should be able to make that more consistent.

The one with the name was even used in the related blog posts:

https://developer.github.com/changes/2016-06-14-repository-invitations/

I hope this answers your question and thanks for reaching out about this!

In my specific case, I'm trying to plug this API into code that only has owner/repo available to it. I have to fetch the repo with Repositories.Get just to get its ID to plug into these functions. I suspect many other implementations have to do the same.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 9, 2017

Hmmm... I'm now wondering if we should have another go generate function that makes these Repo APIs accessible either by "owner/repo" or by "repo_id" (keeping the same name as the original but adding ByID as a suffix to the method name)?

Maybe that is overkill... but I'm not sure I like breaking this API here either when both methods of access should be possible via the API.

Of course, a GraphQL version of the API should solve this, but we aren't there yet.

@dmitshur
Copy link
Member

dmitshur commented Mar 9, 2017

Thanks for emailing them.

As for your question -- that looks like a problem in the documentation. You should definitely be able to use the standard routes like /repos/:owner/:repo/invitations.
...
Not sure why the team decided to document the one with the repository ID instead of the name for this specific endpoint, but we should be able to make that more consistent.

The one with the name was even used in the related blog posts

That's great to hear, and I think that gives us a good chance of being able to fix up this unintended inconsistency in the API.

Hmmm... I'm now wondering if we should have another go generate function that makes these Repo APIs accessible either by "owner/repo" or by "repo_id" (keeping the same name as the original but adding ByID as a suffix to the method name)?

Maybe that is overkill... but I'm not sure I like breaking this API here either when both methods of access should be possible via the API.

Of course, a GraphQL version of the API should solve this, but we aren't there yet.

@gmlewis I think it'd be better to have a discussion about such a large and sweeping potential change in a separate issue. I'm really worried about anything that increases API sizes, and doubling is very significant. That said, I want to refer you to some relevant previous issues on the topic. Consider #68, which I think proposed a very good approach for being able to use both owner/repo and ID-based ways of referring to repositories, users. Unfortunately, it didn't make it in because it was too late to make such a breaking API change.

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 9, 2017

Thanks for the pointer, @shurcooL, I had not seen that. Very clever.
Yes, sounds like fixing the inconsistency is the way to go, then, thus breaking the API.
We could bump the version again.

@dmitshur
Copy link
Member

dmitshur commented Mar 9, 2017

Yes, sounds like fixing the inconsistency is the way to go, then, thus breaking the API.
We could bump the version again.

Let's do that.

Copy link
Member

@dmitshur dmitshur left a comment

Choose a reason for hiding this comment

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

I've reviewed the change, LGTM. Thanks @alindeman.

Let's just bump up the library version number in github.go.

@alindeman
Copy link
Contributor Author

Great, thanks! You all will take the integration from here?

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 10, 2017

It would be helpful if you, @alindeman, bump the library version number in github.go and add a commit to this PR. Thank you!

@alindeman
Copy link
Contributor Author

It would be helpful if you, @alindeman, bump the library version number in github.go and add a commit to this PR. Thank you!

@gmlewis Sure, done.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitshur
Copy link
Member

Ok, merging.

@dmitshur dmitshur merged commit c1bdf18 into google:master Mar 11, 2017
bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017
All repository routes on the GitHub API can accessed via /repositories/:id
and /repos/:owner/:repo. For some reason, the documentation for the
invitation routes only references the former. Contacting GitHub support
has confirmed that the inconsistent documentation was unintentional
and will likely be corrected to use :owner/:repo soon.

Repository invitations are the only functions in google/go-github to use the
:id style routes, so this changes them to use :owner/:repo for consistency,
and as that will likely be much more convenient for users of this library.

Bump version because this is a breaking API change. It affects endpoints
that are part of preview API only.
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.

None yet

4 participants