Skip to content

Merge NestedTeamsPreview branch to master #747

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 6 commits into from
Oct 31, 2017
Merged

Conversation

gmlewis
Copy link
Collaborator

@gmlewis gmlewis commented Oct 9, 2017

No description provided.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Oct 9, 2017

All CLAs signed. Changing label.

@gmlewis gmlewis added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Oct 9, 2017
@gmlewis
Copy link
Collaborator Author

gmlewis commented Oct 9, 2017

libraryVersion needs bumping.

@googlebot googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Oct 9, 2017
@gmlewis gmlewis added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Oct 9, 2017
@elliott-beach
Copy link
Contributor

@gmlewis Would you like me to do that in another PR? I can’t here. I realize now that I could have opened this PR myself.

@googlebot googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Oct 9, 2017
@gmlewis
Copy link
Collaborator Author

gmlewis commented Oct 9, 2017

Thanks @elliotbeach, but I think we are good now. This has been a fun little exercise.
I'm going to ask that @shurcooL review it after I look it over one time, and he can merge if I haven't made any mistakes.

@gmlewis gmlewis added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Oct 9, 2017
@gmlewis gmlewis requested a review from dmitshur October 9, 2017 04:19
Description *string `json:"description,omitempty"`
Maintainers []string `json:"maintainers,omitempty"`
RepoNames []string `json:"repo_names,omitempty"`
ParentTeamID *string `json:"parent_team_id,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub is expecting an integer, not a string. I've opened #748 to address that.

Copy link
Contributor

Choose a reason for hiding this comment

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

To remove a team from the hierarchy and place it at the top level, GitHub allows the PATCH verb to be used with the body {"parent_team_id": null}. Is it possible with omitempty to send this kind of request right now?

But on the other hand, if we remove omitempty naively, we force ParentTeamID to be specified any time we edit the team, even if we only want to edit other attributes.

How do we balance both use cases? Is there prior art?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not possible to specify the id as null with omitempty - similar issues have been encountered in #181 and #236.

Could we add another function, MakeTeamTopLevel, which sets {"parent_team_id": null}?

Copy link
Contributor

Choose a reason for hiding this comment

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

@shurcooL Could we get your opinion on this point?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely similar to #236, thanks for referencing that @elliott-beach.

I suspect we'll need a separate method or something to force sending the null via another internal struct.

@dmitshur
Copy link
Member

dmitshur commented Oct 12, 2017

This is waiting on GitHub indirectly via #748. Edit: Not anymore.

@googlebot googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Oct 13, 2017
@gmlewis gmlewis added cla: yes Indication that the PR author has signed a Google Contributor License Agreement. and removed cla: no labels Oct 13, 2017
@elliott-beach
Copy link
Contributor

elliott-beach commented Oct 15, 2017

I will make a pull request to add a method for setting parent_team_id to null, but I don't see a reason this couldn't be merged now.

@dmitshur
Copy link
Member

dmitshur commented Oct 16, 2017

We'll need to resolve the conflict with libraryVersion bump (it should become 14 now). I'll do that by rebasing this branch on latest master (I hope no one minds, let me know otherwise).

I notice that d51e256 commit message says "Fixes #714.", but I don't see the new endpoint https://developer.github.com/v3/orgs/teams/#list-child-teams implemented yet. Doesn't that mean #714 is not going to be fully resolved yet?

Create mediaTypeNestedTeamsPreview const for
https://developer.github.com/changes/2017-08-30-preview-nested-teams/.

Add parent field to the Team struct.

Set preview API media type in endpoints that return a team, so that
the new parent field gets populated.

Helps #714.
This replaces Team with NewTeam in function that accept a team for
creating or editing a team. The NewTeam struct is similar to the
existing NewPullRequest for creating pull requests.

We need a new struct because the set of parameters for creating or
editing a team contains some parameters that are not in the parameter
for teams, and most properties of teams cannot be used for editing.

The specific change is that Teams are returned with a parent field,
but are created/edited with a parent_id field.

Helps #714.
elliott-beach and others added 3 commits October 16, 2017 01:01
Using the code as written in the `NestedTeamsPreview` branch produces
an error when talking to the GitHub API:

	panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request.

	For 'properties/parent_team_id', "2514824" is not a number or null. []

The documentation at https://developer.github.com/v3/orgs/teams/#create-team
states that this parameter is an id type which appears to mean an integer, not a string.
Also, id is an integer in the sample responses for Get team endpoint, etc.
Change-Id: I522f603a39882f7c1219af2571d616127f1b527f
@googlebot googlebot added cla: no and removed cla: yes Indication that the PR author has signed a Google Contributor License Agreement. labels Oct 16, 2017
@gmlewis
Copy link
Collaborator Author

gmlewis commented Oct 16, 2017

I'm sorry, that Fix message was my fault... I thought it was resolved, but apparently not.

@elliott-beach
Copy link
Contributor

It’s more my fault - I will fix tomorrow.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Oct 26, 2017

@elliott-beach - what is the status here - as far as I can tell, I can squash and merge this, correct?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@elliott-beach
Copy link
Contributor

elliott-beach commented Oct 30, 2017

Now that the ListChildTeams endpoint is added, this should be good.
@shurcooL might want to take another pass on this.

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.

We've reviewed the individual PRs that went into this branch already.

I took a quick general look over everything, didn't spot any issues. LGTM.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Oct 31, 2017

OK, thank you, @shurcooL! I will squash and merge.

@gmlewis gmlewis merged commit 996760c into master Oct 31, 2017
@gmlewis gmlewis deleted the NestedTeamsPreview branch October 31, 2017 16:18
nbareil pushed a commit to nbareil/go-github that referenced this pull request May 1, 2018
* Add support for fetching parent team (for nested teams). (google#723)

Create mediaTypeNestedTeamsPreview const for
https://developer.github.com/changes/2017-08-30-preview-nested-teams/.

Add parent field to the Team struct.

Set preview API media type in endpoints that return a team, so that
the new parent field gets populated.

Helps google#714.

* Add NewTeam struct. (google#724)

This replaces Team with NewTeam in function that accept a team for
creating or editing a team. The NewTeam struct is similar to the
existing NewPullRequest for creating pull requests.

We need a new struct because the set of parameters for creating or
editing a team contains some parameters that are not in the parameter
for teams, and most properties of teams cannot be used for editing.

The specific change is that Teams are returned with a parent field,
but are created/edited with a parent_id field.

Helps google#714.

* Add NestedTeamsPreview header to remaining endpoints

Fixes google#714.

* Change ParentTeamID field to int (from string). (google#748)

Using the code as written in the `NestedTeamsPreview` branch produces
an error when talking to the GitHub API:

	panic: PATCH https://github.com/api/teams/2514825: 422 Invalid request.

	For 'properties/parent_team_id', "2514824" is not a number or null. []

The documentation at https://developer.github.com/v3/orgs/teams/#create-team
states that this parameter is an id type which appears to mean an integer, not a string.
Also, id is an integer in the sample responses for Get team endpoint, etc.

* Bump libraryVersion

* NestedTeamsPreview: Add the ListChildTeams endpoint (google#758)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants