Request parent team from endpoints that return a team by elliott-beach · Pull Request #718 · google/go-github · GitHub
Skip to content

Request parent team from endpoints that return a team#718

Closed
elliott-beach wants to merge 6 commits into
google:masterfrom
elliott-beach:request-parent-team
Closed

Request parent team from endpoints that return a team#718
elliott-beach wants to merge 6 commits into
google:masterfrom
elliott-beach:request-parent-team

Conversation

@elliott-beach

@elliott-beach elliott-beach commented Sep 10, 2017

Copy link
Copy Markdown
Contributor

This PR address part of #714 by

  1. Adding a parent field to the Team struct.
  2. Requesting this field to be populated from each endpoint that returns a team, by adding the API preview header to the request.

Initially, I had added the preview header to the ListTeamMembers, IsTeamMember, and GetTeamMembership endpoints, but dropped those changes because they aren't relevant to the goal of this PR.

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @e-beach!

For each test function that tests one of the methods where you added the new mediaTypeNestedTeamsPreview header, could you please also add a testHeader call in order to be consistent with the rest of the package?

Otherwise, these changes LGTM.

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @e-beach!
LGTM.
Awaiting second LGTM before merging.

@gmlewis gmlewis requested a review from dmitshur September 13, 2017 16:16

@dmitshur dmitshur left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Upon a closer look, I may have to backtrack on something I agreed with earlier in #714 (comment).

I've realized that #714 is not about adding an entirely new API, but making modifications to an existing API.

That means that implementing it piece-by-piece may leave the master branch of go-github in an unusable state. The behavior will not be consistent with previous API (without preview API content-type header), nor with the new preview API (with preview API content-type header). It'll be something weird and hard to understand in between.

For instance, consider this bullet point from the change announcement:

  • Endpoints that return team hashes will now include the parent field representing the parent team. Additionally, the members_count and repositories_count will include all members and repositories according to the nested structure.

Those fields are a part of the Team struct. The endpoints that have the application/vnd.github.hellcat-preview+json header set will return different numbers than other endpoints where application/vnd.github.hellcat-preview+json isn't yet set.

Does that make sense? Do you agree that partially implementing #714 at a time is not a viable approach?

However, I still think keeping PRs small will be helpful; I'd rather not make it so that implementing this feature has to be done all by one person all in one huge PR. I have an alternative idea.

We could create a feature branch and send PRs into that feature branch. Once it has all of #714 implemented, we can merge it into master, therefore implementing #714 on master atomically rather than piece by piece. (Of course, a separate feature branch has downsides of its own, but it seems to be the least evil solution to this challenge. I'm open to feedback and suggestions.) /cc @gmlewis

If you agree, I think we should proceed by closing this PR, creating a feature branch, then asking @e-beach to re-send this PR into that feature branch rather than master.

Comment thread github/orgs_teams_test.go

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor, unneeded blank line here.

@elliott-beach

elliott-beach commented Sep 21, 2017

Copy link
Copy Markdown
Contributor Author

Moving the changes to a separate branch sounds logical to me - it doesn't make sense to add potentially breaking changes piecemeal. The impact of a change such as listing members of child teams, instead of just the parent team, should be handled carefully. A feature branch seems the correct way to group a set of PRs that should be merged collectively.

I guess we are awaiting @gmlewis's opinion, then.

@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Sep 21, 2017
@gmlewis

gmlewis commented Sep 21, 2017

Copy link
Copy Markdown
Collaborator

A separate feature branch seems totally reasonable to me for this use case. I say "go for it".
Thanks!

@dmitshur

Copy link
Copy Markdown
Member

Okay.

I've created a NestedTeamsPreview branch. I'll close this PR, @e-beach please re-send it against that brach rather than master. Feel free to address the one minor review comment I posted (extraneous blank line). Thanks!

@elliott-beach

elliott-beach commented Sep 22, 2017

Copy link
Copy Markdown
Contributor Author

I just discovered that it is possible to edit a PR to be against a different branch.
See: https://stackoverflow.com/q/24159036/5749914
Oh well, I sent a new one.

@dmitshur

Copy link
Copy Markdown
Member

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

Labels

cla: yes Indication that the PR author has signed a Google Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants