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

Add support for the repository transfer API #788

Merged
merged 4 commits into from Nov 23, 2017

Conversation

Projects
None yet
4 participants

juliaferraioli added some commits Nov 16, 2017

Add support for the Repository Transfer API
This API allows for the transfer of one GitHub repository to another
user or organization.

Announcement:
https://developer.github.com/changes/2017-11-09-repository-transfer-api-preview/.
GitHub API Docs:
https://developer.github.com/v3/repos/#transfer-a-repository.

Fixes #777.

@googlebot googlebot added the cla: yes label Nov 16, 2017

@gmlewis

Thank you, @juliaferraioli!
LGTM.
@shurcooL and/or @elliott-beach can review, then @shurcooL can merge.

@dmitshur

I have some comments/questions about the API design first. The rest looks excellent from a cursory look so far.

// TransferRequest represents a request to transfer a repository.
type TransferRequest struct {
NewOwner string `json:"new_owner,omitempty"`

This comment has been minimized.

@dmitshur

dmitshur Nov 18, 2017

Member

According to GitHub API docs, new_owner is required, so I don't think omitempty is appropriate here.

This comment has been minimized.

@juliaferraioli

juliaferraioli Nov 20, 2017

Member

Excellent catch. Fixed.

// in a successful request.
//
// GitHub API docs: https://developer.github.com/v3/repos/#transfer-a-repository
func (s *RepositoriesService) Transfer(ctx context.Context, owner, repo string, transfer *TransferRequest) (*Repository, *Response, error) {

This comment has been minimized.

@dmitshur

dmitshur Nov 18, 2017

Member

Why is transfer a *TransferRequest pointer rather than value?

That means user can pass nil, but that wouldn't be meaningful, would it?

This comment has been minimized.

@juliaferraioli

juliaferraioli Nov 20, 2017

Member

I understand your point here. And while I tend to agree, there's precedent for this approach even in repos.create being passed a pointer to a Repo. I didn't want to introduce code that broke with that model.

Thoughts?

This comment has been minimized.

@dmitshur

dmitshur Nov 22, 2017

Member

there's precedent for this approach even in repos.create being passed a pointer to a Repo. I didn't want to introduce code that broke with that model.

I see. However, that API decision was made 5 years ago, and is not very representative of the best practices regarding API decisions this repo has evolved toward since that time.

In fact, we've ran into possible panics in the library caused due to such parameters being passed as pointers in situations where nil values made no sense. We dealt with that in #531 and #539, by adding if param == nil { return errors.New(...) } guards.

We went that route because it was less disruptive than breaking an API for a very minor improvement. Given two equivalent choices, we prefer consistency.

However, in this case, it's a new API, so we can go with the best API design we can come up with. Using a pointer isn't meaningful and doesn't add value (does it?), but it create a liability for dealing with users potentially passing nil values.

I understand your point about wanting to be consistent with existing code, but in this case, I don't think the consistency argument applies, because using a pointer has tangible downsides compared to using a value. They're not equivalent choices. When adding new APIs to this library, we try to follow the best known practices, and we sometimes even break APIs of preview endpoints when improvements are discovered.

If you want to see more recent precedent, see #773 which came up during review of #771. The original version started by using pointers, but we came to agreement that value parameters were better.

(One could argue that TransferRequest contains just 2 fields that could be inlined into the method signature. However, I think using a dedicated struct for it as you've done makes sense, because it maps more closely to GitHub API as documented, and will be easy to update should GitHub add extra fields to that input object.)

What do you think?

This comment has been minimized.

@juliaferraioli

juliaferraioli Nov 22, 2017

Member

That sounds reasonable to me. Thanks for the thorough background! I'll go ahead and work on the updates today.

I had thought about your parenthetical initially, but decided against it for exactly the reasons you enumerated.

@dmitshur

LGTM. Thank you for contributing @juliaferraioli!

@dmitshur dmitshur merged commit 4143024 into google:master Nov 23, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018

// TransferRequest represents a request to transfer a repository.
type TransferRequest struct {
NewOwner string `json:"new_owner"`
TeamID []int `json:"team_id,omitempty"`

This comment has been minimized.

@dmitshur

dmitshur Jul 11, 2018

Member

I just noticed now, I think the JSON name team_id should've been team_ids, isn't that right?

From https://developer.github.com/v3/repos/#transfer-a-repository:

image

Unless GitHub API documentation is incorrect. I don't have any evidence suggesting that.

(FWIW, https://developer.github.com/v3/orgs/members/#create-organization-invitation also uses team_ids.)

This comment has been minimized.

@gmlewis

gmlewis Jul 12, 2018

Collaborator

Good catch, @shurcooL!

This comment has been minimized.

@juliaferraioli

juliaferraioli Jul 12, 2018

Member

Yikes! Nice catch indeed. I can prepare a pr tomorrow (unless you already have one). I'm afk for the rest of tonight.

This comment has been minimized.

@gmlewis

gmlewis Jul 12, 2018

Collaborator

Thank you, @juliaferraioli! No rush... this is not urgent.

dmitshur added a commit that referenced this pull request Jul 12, 2018

Use "team_ids" JSON name for TransferRequest.TeamID field (#947)
According to GitHub API docs at https://developer.github.com/v3/repos/#transfer-a-repository,
the field name should be "team_ids". This change updates it to match.

Fixes #946.
Updates #788.

gmlewis added a commit to gmlewis/go-github that referenced this pull request Aug 10, 2018

Use "team_ids" JSON name for TransferRequest.TeamID field (google#947)
According to GitHub API docs at https://developer.github.com/v3/repos/#transfer-a-repository,
the field name should be "team_ids". This change updates it to match.

Fixes google#946.
Updates google#788.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment